-
Notifications
You must be signed in to change notification settings - Fork 704
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
Pipeline template feature flag / 'ValidForSpinnakerVersion' annotation. #637
Conversation
d22663c
to
a7c28a3
Compare
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.
LGTM - Few comments, and have you verified that the bindings won't crash when settings.js is at a non-compatible version?
@@ -62,6 +69,11 @@ protected void executeThis() { | |||
|
|||
features.setChaos(chaos != null ? chaos : features.isChaos()); | |||
features.setJobs(jobs != null ? jobs : features.isJobs()); | |||
if (!pipelineTemplates) { |
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.
If this var is null
I think java will crash here with an NPE
@@ -43,6 +43,7 @@ public NodeIterator getChildren() { | |||
private boolean chaos; | |||
private boolean entityTags; | |||
private boolean jobs; | |||
@ValidForSpinnakerVersion(lowerBound = "1.2.0") private Boolean pipelineTemplates; |
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.
Nice
|
||
return runMatchingValidators(psBuilder, validator, node, c.getSuperclass()) + result; | ||
|
||
if (result > 0) { |
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.
Given that result can be either 1 or 0 and nothing else I think a bool is more appropriate here. Returning a bool makes more sense then too.
if (fieldNotValid) { | ||
p.addProblem(Problem.Severity.WARNING, "Field " + clazz.getSimpleName() + "." + field.getName() | ||
+ " not supported for Spinnaker version " + spinnakerVersion + "." | ||
+ " Use at least " + annotation.lowerBound() + "."); |
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.
This last "help" text should be set in .setRemediation(String)
which can be called on the result of .addProblem()
Yes, verified that settings.js is written properly for 1.1.2. |
a7c28a3
to
4d33a48
Compare
Addressed comments. |
3434d2b
to
3c57106
Compare
3c57106
to
de9201e
Compare
@lwander please review.