Skip to content

[ts] Update network-facing libs like express (et al.), node-fetch, p-… #8312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 21, 2022

Conversation

geropl
Copy link
Member

@geropl geropl commented Feb 18, 2022

…throttle, probot

Description

Besides the update, this PR sets explicit timeouts on all fetch calls. Hope this did not cause our increasing lag: firewalls/rate-limiter for github.com DROP ing our packets might very well have caused this behavior. 🙈

This took a bit longer than expected: We have (by design) divergence how our tests are executed (leeway vs yarn test), and that broke here.

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

@geropl geropl requested a review from a team February 18, 2022 13:12
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 18, 2022
JanKoehnlein
JanKoehnlein previously approved these changes Feb 18, 2022
Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

JanKoehnlein
JanKoehnlein previously approved these changes Feb 18, 2022
Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@geropl geropl force-pushed the gpl/update-ts-deps branch 2 times, most recently from b33eef8 to dc6fb08 Compare February 21, 2022 11:02
@roboquat roboquat added size/L and removed size/XL labels Feb 21, 2022
@geropl
Copy link
Member Author

geropl commented Feb 21, 2022

/werft run

👍 started the job as gitpod-build-gpl-update-ts-deps.5

🤞 that certs work now

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #8312 (bd595fa) into main (6f8bbba) will increase coverage by 2.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            main    #8312      +/-   ##
=========================================
+ Coverage   8.42%   11.17%   +2.75%     
=========================================
  Files         33       18      -15     
  Lines       2339      993    -1346     
=========================================
- Hits         197      111      -86     
+ Misses      2137      880    -1257     
+ Partials       5        2       -3     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-installer-raw-app ?
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/installer/pkg/common/storage.go
components/installer/pkg/common/render.go
...components/ws-manager/unpriviledged-rolebinding.go
components/installer/pkg/common/common.go
...s/installer/pkg/components/ws-manager/configmap.go
components/installer/pkg/common/objects.go
...installer/pkg/components/ws-manager/rolebinding.go
.../installer/pkg/components/ws-manager/deployment.go
components/local-app/pkg/auth/auth.go
components/installer/pkg/common/ca.go
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f8bbba...bd595fa. Read the comment docs.

@geropl geropl requested a review from JanKoehnlein February 21, 2022 12:39
@geropl
Copy link
Member Author

geropl commented Feb 21, 2022

Finally green - sorry for the noise here.

This took a bit longer than expected: We have (by design) divergence how our tests are executed (leeway vs yarn test), and that broke here.

"opentracing": "^0.14.4",
"p-throttle": "^3.0.0",
"p-throttle": "^5.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require() of ES Module /app/node_modules/p-throttle/index.js

same issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"type": "module" in package.json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require() of ES Module /app/node_modules/p-throttle/index.js

same issue

What did you do? And why did the build run? 😬

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

server is crashing in the preview deployment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"type": "module" in package.json?

Not, it's not that easy. Maybe not too complicated either, but I want the other changes to be in by today. I will remove pthrottle, then.

Can you give be your cmdline to reproduce?

@geropl geropl force-pushed the gpl/update-ts-deps branch from dc6fb08 to bd595fa Compare February 21, 2022 16:48
Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good! Thanks

(image pull issues in preview env are not related.)

@roboquat roboquat merged commit a81c23e into main Feb 21, 2022
@roboquat roboquat deleted the gpl/update-ts-deps branch February 21, 2022 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants