Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TEP-0061, Allow custom task to be embedded. #3901
TEP-0061, Allow custom task to be embedded. #3901
Changes from all commits
d779a31
dcdfe07
239f98c
4e7a9ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
need an additional check here, if both are
nil
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.
Hi @pritidesai Thanks for observing !
The check above this covers the case if both are nil. Infact, previously when I was testing with the additional explicit check of both nil. The code was just unreachable. So
equality.Semantic.DeepEqual(rs, &RunSpec{})
take care of both nil case as well. For the same reason, it is not possible to write a separate test.Matter of factly, There is a comment at
line 39
, just above the check explaining the same.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.
thanks @ScrapCodes
right but the
DeepEqual
used this way checks if thers
is empty, for example:pipeline/pkg/apis/pipeline/v1beta1/pipeline_validation.go
Lines 45 to 47 in 4ced3cb
This check will not catch
rs.Spec
andrs.Ref
both being nil ifrs
had any other field initialized. Since the run specifications are generated by the controller, this is not blocking this PR but since run specifications are being consumed by the custom controller, it will be great to have such validation here.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.
Thanks, now I have understood ! Will open a PR with the fix soon.
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.
=> empty ref and spec
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.
the test name does not match with ref and spec both being
nil
, please add one more check in the validation routine and update this test.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.
The reason is, both nil and
Empty spec
are same in terms of what happens underneath.Actual check says,
if equality.Semantic.DeepEqual(rs, &RunSpec{}) { return apis.ErrMissingField("spec")
i.e. empty spec and so is the name. Please see the reply to your comment above as well, and let me know, if this is still concerning.Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.