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

Allow simultaneous upgrade and parameter update #1495

Merged
merged 1 commit into from
May 7, 2020

Conversation

zen-dog
Copy link
Contributor

@zen-dog zen-dog commented May 6, 2020

if the deploy plan is triggered by the update. This seems to be a reasonable exception since the deploy plan is used for upgrades in many cases anyway.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

if `deploy` plan is triggered by the update. This seems to be a reasonable exception, since `deploy` plan is used for upgrades in many cases anyway.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Copy link
Member

@ANeumann82 ANeumann82 left a comment

Choose a reason for hiding this comment

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

lgtm, although I think we may need to clear up all the rules around param triggers and OV upgrades and enfore them in package verify

Comment on lines 41 to 42
Name: "bar",
Trigger: update,
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually allowed? I thought an update is only performed if the OV changes, and doesn't have anything to do with param changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we established these rules. It's all a bit fuzzy and determined by some old code. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Then we should establish some rules to remove the fuzziness. We shouldn't have legacy code that drives behavior we don't fully understand in a project that's still in beta.

Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

LGTM! Let's also document these behaviors in https://kudo.dev/docs/developing-operators/plans.html.

Comment on lines 41 to 42
Name: "bar",
Trigger: update,
Copy link
Member

Choose a reason for hiding this comment

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

Then we should establish some rules to remove the fuzziness. We shouldn't have legacy code that drives behavior we don't fully understand in a project that's still in beta.

@zen-dog
Copy link
Contributor Author

zen-dog commented May 7, 2020

Then we should establish some rules to remove the fuzziness. We shouldn't have legacy code that drives behavior we don't fully understand in a project that's still in beta.

I agree. Like I don't quite buy the special update plan. Does it really need to be a special case? But let's do it in a separate effort.

@zen-dog zen-dog merged commit 58ef809 into master May 7, 2020
@zen-dog zen-dog deleted the ad/upgrade-with-update branch May 7, 2020 11:24
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.

None yet

3 participants