-
Notifications
You must be signed in to change notification settings - Fork 881
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 legacyRemotingSecurityEnabled
flag
#625
Conversation
url: https://RELEASE-NAME-jenkins:8080 | ||
|
||
- it: legacyRemotingSecurityEnabled = false (even though jenkins is 2.325 or older) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if the tag is not a semver tag and legacyRemotingSecurityEnabled
is not set?
I can't easily tell from reading the code.
(would be good if there was a test showing what it does)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if the tag is not a semver tag and legacyRemotingSecurityEnabled is not set?
It will error out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems Helm does not have a nice way of handling this error or providing a nicer message... but adding that regex thing from #623 is also a bit overkill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just back out the semver thing.
Default this legacy flag to false, document that people on x version or older will need to enable this flag if they want to keep it enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I don't mind doing that, but that means a breaking change and a major version bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no issue with doing that. we should make changes as required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I got it right in the end. The signoff is killing me :) and the whitespace in the tests... Shall I open a PR to try to bump helm to latest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll open a separate PR once this one is merged, thanks
Signed-off-by: Dominykas Blyžė <[email protected]>
Signed-off-by: Dominykas Blyžė <[email protected]>
Signed-off-by: Dominykas Blyžė <[email protected]>
Signed-off-by: Dominykas Blyžė <[email protected]>
Signed-off-by: Dominykas Blyžė <[email protected]>
Probably, at work we had some test failures updating to 3.8, something got stricter can't remember the specific issue |
legacyRemotingSecurityEnabled
flag
BREAKING CHANGES: removes the `remotingSecurity` flag based on `controller.tag` Signed-off-by: Dominykas Blyžė <[email protected]>
Signed-off-by: Dominykas Blyžė <[email protected]>
@@ -1447,102 +1447,201 @@ tests: | |||
location: | |||
adminAddress: | |||
url: https://RELEASE-NAME-jenkins:8080 | |||
- it: default config for jenkins 2.325 or older | |||
- it: legacyRemotingSecurityEnabled = false (even though jenkins is 2.325 or older) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(probably don't commit from github due to signoff requirements...)
- it: legacyRemotingSecurityEnabled = false (even though jenkins is 2.325 or older) | |
- it: legacyRemotingSecurityEnabled = false |
Signed-off-by: Dominykas Blyžė <[email protected]>
charts/jenkins/CHANGELOG.md
Outdated
@@ -12,6 +12,10 @@ Use the following links to reference issues, PRs, and commits prior to v2.6.0. | |||
The change log until v1.5.7 was auto-generated based on git commits. | |||
Those entries include a reference to the git commit to be able to get more details. | |||
|
|||
## 4.0.0 | |||
|
|||
Removes automatic `remotingSecurity` setting when using a container tag older than `2.236` (introduced in [`3.11.7`](#3117)). If you're using a version older than `2.236`, you should explicitly set `.controller.legacyRemotingSecurityEnabled` to `true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes automatic `remotingSecurity` setting when using a container tag older than `2.236` (introduced in [`3.11.7`](#3117)). If you're using a version older than `2.236`, you should explicitly set `.controller.legacyRemotingSecurityEnabled` to `true`. | |
Removes automatic `remotingSecurity` setting when using a container tag older than `2.326` (introduced in [`3.11.7`](#3117)). If you're using a version older than `2.326`, you should explicitly set `.controller.legacyRemotingSecurityEnabled` to `true`. |
Signed-off-by: Dominykas Blyžė <[email protected]>
What this PR does / why we need it
remotingSecurity
#616Also updates whitespace in the unit tests - they were failing for me locally. Do they even get executed in CI? I don't see anything in the logs (even though the command is mentioned inIt seems they're only failing for me locally... Is it due to helm version mismatch or smth? Seems CI is running 3.4, while I'm running 3.8...ct.yaml
)?Which issue this PR fixes
remotingSecurity
#616Special notes for your reviewer
Checklist