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: only set restricted field to default value if none of the conditionals set it #284

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

backjo
Copy link
Collaborator

@backjo backjo commented Apr 29, 2021

fixes #283

Signed-off-by: Jonah Back jonah@jonahback.com

…ionals set it

Signed-off-by: Jonah Back <jonah@jonahback.com>
@backjo backjo requested review from a team as code owners April 29, 2021 16:57
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #284 (eb88d4f) into master (002bf72) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #284   +/-   ##
=======================================
  Coverage   70.23%   70.23%           
=======================================
  Files          19       19           
  Lines        2970     2970           
=======================================
  Hits         2086     2086           
  Misses        755      755           
  Partials      129      129           
Impacted Files Coverage Δ
controllers/provisioners/config.go 78.34% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 002bf72...eb88d4f. Read the comment docs.

@eytan-avisror
Copy link
Collaborator

So this would set all conditional with the first match?
Would we need a hotfix btw?

@backjo
Copy link
Collaborator Author

backjo commented Apr 29, 2021

Worse, actually. It would only set fields that were in the last match, since setField was getting reset for every applicable conditional. If the field wasn't set by the last conditional, it would override with the default value, if one existed.

@backjo
Copy link
Collaborator Author

backjo commented Apr 29, 2021

Yeah, a patch release would probably make sense.

@backjo backjo merged commit 59dd8b1 into master Apr 29, 2021
@backjo backjo deleted the fix/conditional branch April 29, 2021 17:09
@backjo backjo mentioned this pull request Apr 29, 2021
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.

Conditional restricted fields are not applied if the last applicable rule did not set the field.
2 participants