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

Warn user when using lowercase secrets #4218

Open
wants to merge 3 commits into
base: release/v2.7
Choose a base branch
from

Conversation

zc-devs
Copy link
Contributor

@zc-devs zc-devs commented Oct 10, 2024

Follow up #3960 (comment)

Lints

Removed uppercasing all secret env vars, instead, the value of the secrets property is used

@qwerty287
Copy link
Contributor

What does "it might not work properly" work?

It should work. You just have to access it with the lower case name as you defined it.

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 10, 2024

it might not work properly = you have to access it with the lower case name or change secret name in pipeline

Fixed message in b5cf0d0

@anbraten anbraten changed the title Lint secrets case Warn user when using lowercase secrets Oct 10, 2024
@6543
Copy link
Member

6543 commented Oct 11, 2024

?!? did we not move to case sensitive iand depricate the secret key as a whole ?!?

@zc-devs
Copy link
Contributor Author

zc-devs commented Oct 12, 2024

move to case sensitive

Yes, therefore the PR.

depricate the secret key as a whole

No, there was empty deprecation section in the linter code on main when I made the PR and there was no warnings in UI on next and 2.7 in this regard.
It is being discussed #4015.

@qwerty287 qwerty287 mentioned this pull request Oct 20, 2024
@qwerty287
Copy link
Contributor

So while I understand that you would like to have a warning, this is an enhancement and therefore shouldn't go into 2.7.x.

@lafriks
Copy link
Contributor

lafriks commented Oct 21, 2024

I don't think this is needed as we already will have warning in 3.0 about deprecated secrets section as whole

@qwerty287
Copy link
Contributor

Now with #4235, I agree to @lafriks. However, we plan to do a 2.8.0 release with some important enhancements. We can put this on the list for it and merge it there after we did 2.7.2

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

Successfully merging this pull request may close these issues.

4 participants