Skip to content
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

Add env var name/value length validation #8248

Merged
merged 2 commits into from
May 6, 2022

Conversation

randomir
Copy link
Contributor

@randomir randomir commented Feb 16, 2022

Redo #8046, this time with shorter branch name that werft can digest.

Fixes #8045.

Release Notes

Add user environment variable name length and value length validation in settings UI modal.

@jankeromnes
Copy link
Contributor

jankeromnes commented Feb 16, 2022

Thanks a lot for making Werft happy!

CLA is still in progress: #8046 (comment)

/werft run

👍 started the job as gitpod-build-env-var-len-validation-fork.0

jankeromnes
jankeromnes previously approved these changes Feb 16, 2022
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Thank you again for this fix @randomir!

The code changes look good and work as advertized. 🎯 I've also added a few minor thoughts in-line.

Adding a manual hold on the auto-merger until the CLA is confirmed:

/hold

@randomir
Copy link
Contributor Author

randomir commented Feb 16, 2022

Thanks @jankeromnes for the quick review. Now we wait for the CLA... 📜

@stale
Copy link

stale bot commented Feb 27, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Feb 27, 2022
@JanKoehnlein JanKoehnlein added meta: never-stale This issue can never become stale and removed meta: stale This issue/PR is stale and will be closed soon labels Feb 28, 2022
@geropl
Copy link
Member

geropl commented Mar 2, 2022

@meysholdt Any updates wrt the CLA? (pinged because previous comments: here and here)

@meysholdt
Copy link
Member

I've pinged them again via email about the CLA.

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Still looks good to me 🎉 😁

@jankeromnes
Copy link
Contributor

jankeromnes commented Apr 1, 2022

/werft run

👍 started the job as gitpod-build-env-var-len-validation-fork.1

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #8248 (9cc9a32) into main (a567595) will increase coverage by 3.51%.
The diff coverage is n/a.

❗ Current head 9cc9a32 differs from pull request most recent head 72ad89c. Consider uploading reports for the commit 72ad89c to get more accurate results

@@            Coverage Diff            @@
##            main    #8248      +/-   ##
=========================================
+ Coverage   7.34%   10.86%   +3.51%     
=========================================
  Files         32       18      -14     
  Lines       2234     1022    -1212     
=========================================
- Hits         164      111      -53     
+ Misses      2067      909    -1158     
+ Partials       3        2       -1     
Flag Coverage Δ
components-gitpod-cli-app 10.86% <ø> (-0.32%) ⬇️
install-installer-raw-app ?

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

Impacted Files Coverage Δ
components/gitpod-cli/cmd/credential-helper.go 5.58% <0.00%> (-1.09%) ⬇️
...nstall/installer/pkg/components/ws-manager/role.go
...l/installer/pkg/components/ws-manager/configmap.go
install/installer/pkg/common/render.go
install/installer/pkg/common/common.go
install/installer/pkg/common/ca.go
install/installer/pkg/common/storage.go
install/installer/pkg/common/display.go
.../installer/pkg/components/ws-manager/deployment.go
install/installer/pkg/common/objects.go
... and 5 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@geropl
Copy link
Member

geropl commented Apr 7, 2022

@jankeromnes assigning you because you last approved 😉

@geropl
Copy link
Member

geropl commented Apr 21, 2022

Closing this for now, as we're waiting for a respond regarding CLA. We can always re-open. 🙏

@geropl geropl closed this Apr 21, 2022
@meysholdt
Copy link
Member

we got the CLA signed with D-Wave!

@meysholdt meysholdt reopened this May 4, 2022
@meysholdt meysholdt removed the do-not-merge/cla-pending CLA has not been signed label May 4, 2022
@randomir
Copy link
Contributor Author

randomir commented May 4, 2022

I'll rebase shortly.

randomir and others added 2 commits May 4, 2022 10:01
Also, decrease maxlen for value from ~64k*3/4 to a nice round 32k.
Co-authored-by: Jan Keromnes <janx@linux.com>
@randomir randomir force-pushed the env-var-len-validation branch from 9cc9a32 to 72ad89c Compare May 4, 2022 17:04
@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

/werft run

👍 started the job as gitpod-build-env-var-len-validation-fork.2
(with .werft/ from main)

@gtsiolis gtsiolis requested a review from a team May 6, 2022 14:54
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Many thanks for going through the long CLA process and for rebasing this PR!

Also, thanks George for triggering the new Werft job!

Re-approved and good to go 🚀

@randomir
Copy link
Contributor Author

randomir commented May 6, 2022

Thanks @jankeromnes and @gtsiolis. Sorry it took longer than expected to get the CLA approved. Luckily, it's a one-off thing. 😄

@jankeromnes
Copy link
Contributor

/unhold

@roboquat roboquat merged commit ce5b6cd into gitpod-io:main May 6, 2022
@jankeromnes
Copy link
Contributor

jankeromnes commented May 6, 2022

Thanks a lot for improving Gitpod! 💯 🙏

@randomir randomir deleted the env-var-len-validation branch May 6, 2022 15:25
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: accepted ✔️ community-contribution deployed: webapp Meta team change is running in production deployed Change is completely running in production meta: never-stale This issue can never become stale release-note size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User environment variable name/value length validation missing
7 participants