Skip to content

Add environment variable name/value length validation in UI #8046

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

Conversation

randomir
Copy link
Contributor

@randomir randomir commented Feb 4, 2022

Also, decrease maxlen for value from ~64k*3/4 to a nice round 32k.

Description

On variable save, we now validate:

  • name len is shorter than 256
  • value len is shorter than 32k+1

and inform the user of the error, instead of silently failing.

The 32k limit might be worth some discussion. The db limit is mysql's TEXT max of 64k, but that's after encryption and base64 encoding, resulting with a real current limit of about ~48k. I chose 32k because it looks nicer 😆.

Related Issue(s)

Fixes #8045

How to test

  1. Try to create a variable with either name longer than 255 chars or value longer than 32768 chars.
  2. Create a new variable, then try updating the name and value with strings longer than the limits above.

Release Notes

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

Documentation

No update.

Also, decrease maxlen for value from ~64k*3/4 to a nice round 32k.
@randomir randomir requested a review from a team February 4, 2022 21:40
@roboquat roboquat added release-note team: webapp Issue belongs to the WebApp team size/XS labels Feb 4, 2022
@roboquat
Copy link
Contributor

roboquat commented Feb 6, 2022

@AmirHosseinKarimi: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@roboquat
Copy link
Contributor

roboquat commented Feb 6, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AmirHosseinKarimi
To complete the pull request process, please assign geropl after the PR has been reviewed.
You can assign the PR to them by writing /assign @geropl in a comment when ready.

Associated issue: #8045

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@AlexTugarev
Copy link
Member

AlexTugarev commented Feb 7, 2022

/werft run

👍 started the job as gitpod-build-add-env-var-length-validation-issue-8045-fork.0

@AlexTugarev
Copy link
Member

AlexTugarev commented Feb 7, 2022

Unfortunately, the deployment of a preview environment is failing because the name of the branch is too long. (k8s namespace is derived from the branch name.)

On the other hand, it's sufficient to check for a build in a workspace. (✅ )

@AlexTugarev
Copy link
Member

AlexTugarev commented Feb 7, 2022

@meysholdt, we need you help with a CLA, please! 🙏🏻

@gtsiolis gtsiolis added the do-not-merge/cla-pending CLA has not been signed label Feb 7, 2022
@meysholdt
Copy link
Member

hi @randomir! Great to see a PR from you :-D I've reached out to you via email about the CLA.

@jankeromnes
Copy link
Contributor

jankeromnes commented Feb 14, 2022

/werft run

👍 started the job as gitpod-build-add-env-var-length-validation-issue-8045-fork.1

@jankeromnes
Copy link
Contributor

jankeromnes commented Feb 16, 2022

Well, thank you for nothing, Werft 😅

CI job failed with:

Error: there is no subdomain + 'add-env-var-length-validation-issue-8045-fork.staging.gitpod-dev.com' shorter or equal to 63 characters, max. allowed length for CN.
No HTTPS certs for you! Consider using a short branch name...

I guess we'll need to move this commit to a branch with a shorter name. Let's do that after the CLA question is resolved.

@randomir randomir closed this Feb 16, 2022
@randomir randomir deleted the add-env-var-length-validation/issue-8045 branch February 16, 2022 15:48
@randomir
Copy link
Contributor Author

randomir commented Feb 16, 2022

Thanks @jankeromnes.

I've tried renaming the branch on GH, but that just closed the PR. I've opened a new PR from a shorter branch name in #8248.

I'm fine with that one lingering until the CLA is signed.

@jankeromnes
Copy link
Contributor

jankeromnes commented Feb 16, 2022

Aha, that's perfect, many thanks @randomir! 🙏

Then I suggest we move over to the new Pull Request. 🚚

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User environment variable name/value length validation missing
7 participants