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

Fix issue 592 by adding a regexMatch condition #623

Closed
wants to merge 2 commits into from

Conversation

stijnbrouwers
Copy link

Signed-off-by: Stijn Brouwers [email protected]

What this PR does / why we need it

Fixes issue 592 by first applying a regex match to the official semver regex (https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string) with a small modification: The patch part of the regex had to be made optional for this to work.

Which issue this PR fixes

Special notes for your reviewer

Checklist

  • [ X ] DCO signed
  • [ X ] Chart Version bumped
  • [ X ] CHANGELOG.md was updated

@stijnbrouwers stijnbrouwers requested a review from a team as a code owner May 13, 2022 09:57
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

what about just removing remotingSecurity: enabled: true completely?

People on an older version can re-add it themselves if they want to keep it but we should remove it from the defaults?


This regex is quite complicated and will still fail if people don't include the version

@stijnbrouwers
Copy link
Author

For me that sounds good.
I wasn't sure if I could just delete it since I don't really know the impact of it...
I just saw this issue: #616
If this get's introduced and defaulted to false, it will also solve the issue.

@timja
Copy link
Member

timja commented May 13, 2022

I just saw this issue: #616

Ah I hadn't seen that, yes that would work and it should be defaulted to not including this

@stijnbrouwers
Copy link
Author

OK, that's better. Shall I just close this PR without merging then?

@timja timja closed this May 13, 2022
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.

Possibility of semver mismatch
2 participants