interface: beforeStageTask for updateRun #308
Conversation
Arvindthiru
force-pushed
the
preStageApprovalAPI
branch
2 times, most recently
from
de99f04 to
6b7e498
Compare
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
Arvindthiru
force-pushed
the
preStageApprovalAPI
branch
from
6b7e498 to
64b3042
Compare
Arvindthiru
changed the title
| // +kubebuilder:validation:Enum=TimedWait;Approval | ||
| // +kubebuilder:validation:Required | ||
| Type AfterStageTaskType `json:"type"` | ||
| Type StageTaskType `json:"type"` |
There was a problem hiding this comment.
Overall LGTM just worried about the name change. Wont existing updateRuns run into issues with this change?
There was a problem hiding this comment.
we didn't change the json name "type". I verified in the CRD definition
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| // The task that needs to be completed successfully before starting the stage. | ||
| // +kubebuilder:validation:Optional | ||
| BeforeStageTask *BeforeStageTask `json:"beforeStageTask,omitempty"` |
There was a problem hiding this comment.
is it possible to have more than one task?
There was a problem hiding this comment.
It is possible in the future, hence made BeforeStageTasks a list but we only support Approval as a BeforeStageTask so we currently only support one item within the list
There was a problem hiding this comment.
They are in as part of this commit d9bc4c7
| } | ||
|
|
||
| // BeforeStageTask is the pre-stage task that needs to be completed before starting the stage. | ||
| type BeforeStageTask struct { |
There was a problem hiding this comment.
do we need to create a different type?
There was a problem hiding this comment.
Changed to StageTask for both BeforeStageTask, AfterStageTask
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
Signed-off-by: Arvind Thirumurugan <arvindth@microsoft.com>
| // +kubebuilder:validation:Enum=TimedWait;Approval | ||
| // +kubebuilder:validation:Required | ||
| Type AfterStageTaskType `json:"type"` | ||
| Type StageTaskType `json:"type"` |
There was a problem hiding this comment.
we didn't change the json name "type". I verified in the CRD definition
serbrech
left a comment
serbrech
left a comment
There was a problem hiding this comment.
I think it would really help if you clearly outlined in the PR description what APIs you are changing, what is the behavior expectation with existing data.
how defaulting and validation is handled, instead of leaving that to the reviewer.
It's not clear if what I infered from reading the code is the intended behavior.
Same with the change of type and why it's fine/what are the implications
| type StageTask struct { | ||
| // The type of the before or after stage task. | ||
| // +kubebuilder:validation:Enum=TimedWait;Approval | ||
| // +kubebuilder:validation:Required |
There was a problem hiding this comment.
How do you handle the fact that you are adding a new property that is required?
I would expect this to fail get+re-put of existing updateruns?
at least any client that does not bump the SDK will fail manipulating the updatrun object, as it won't be able to serialize the Type property?
Is there a way to configure defaults on added properties? is there a way to test this?
There was a problem hiding this comment.
I think that this will work as existing updateruns would have no value, so it will get a default value on read of "TimedWait", which is the correct expectation.
new updateruns are required to specify the type because of validation.
There was a problem hiding this comment.
This ia an existing object I'm re-naming for re-use
There was a problem hiding this comment.
the Type property on the object is new, and is required.
4 participants
Description of your changes
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer