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

updog: Make check-update wave aware #615

Merged
merged 1 commit into from
Feb 17, 2020
Merged

updog: Make check-update wave aware #615

merged 1 commit into from
Feb 17, 2020

Conversation

sam-aws
Copy link
Contributor

@sam-aws sam-aws commented Dec 20, 2019

Issue #, if available:
N/A

Description of changes:
Change the behaviour of check-update to not return any update unless the
wave for the update that Updog is in is also ready.
If no update is available or ready, Updog will now also return an error
instead of succeeding silently.

Signed-off-by: Samuel Mendoza-Jonas samjonas@amazon.com

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jhaynes
Copy link
Contributor

jhaynes commented Dec 23, 2019

While wave awareness should be default, I think we'll want to add a flag to updog (or somewhere similar) so an administrator can manually trigger an update ignoring waves if they want to. Thoughts?

@sam-aws
Copy link
Contributor Author

sam-aws commented Dec 24, 2019

While this technically doesn't change that (updog update --now still works for example), I agree that the same should apply for check-update; either having --now apply too, or check-update --any..

@sam-aws
Copy link
Contributor Author

sam-aws commented Dec 26, 2019

Updated so that check-update --now ignores whether the associated wave is ready or not.

workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
@@ -336,6 +337,26 @@ fn update_flags() -> Result<()> {
Ok(())
}

/// List any available update that matches the current flavor, ignoring waves
fn list_updates(manifest: &Manifest, flavor: &str, json: bool) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

This, and other "flavor" references, should be updated to reflect the usage of "variant" vs "flavor".

Copy link
Member

Choose a reason for hiding this comment

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

Chatted offline, we're holding off for a dedicated changeset!

Copy link
Member

Choose a reason for hiding this comment

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

nit: It looks like the args in applicable_updates were updated to use variant, should this usage also then be updated to match?

workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
@sam-aws sam-aws force-pushed the no-updates-here branch 2 times, most recently from fbb3375 to 6a2ae66 Compare February 6, 2020 01:02
@sam-aws
Copy link
Contributor Author

sam-aws commented Feb 6, 2020

Updated to remove that stray .unwrap, and s/ignore-wave/ignore-waves as it's more natural.

workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
workspaces/updater/updog/src/main.rs Show resolved Hide resolved
workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
@sam-aws
Copy link
Contributor Author

sam-aws commented Feb 10, 2020

Updated to use "ignore_waves" consistently, update the help text for update --now, and consolidate the two places handling the printing of the flavor-version-datastore triplet.

@sam-aws
Copy link
Contributor Author

sam-aws commented Feb 17, 2020

Rebased and fixed up some trailing "flavor" usage.

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

Changes LGTM, albeit with one nit-picky question on flavor/variant usage.

@@ -336,6 +337,26 @@ fn update_flags() -> Result<()> {
Ok(())
}

/// List any available update that matches the current flavor, ignoring waves
fn list_updates(manifest: &Manifest, flavor: &str, json: bool) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: It looks like the args in applicable_updates were updated to use variant, should this usage also then be updated to match?

Change the behaviour of check-update to not return any update unless the
wave for the update that Updog is in is also ready.
If no update is available or ready, Updog will now also return an error
instead of succeeding silently.

Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
@sam-aws
Copy link
Contributor Author

sam-aws commented Feb 17, 2020

@jahkeup good catch, fixed up!

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

🚢 🚀

@sam-aws sam-aws merged commit e2e11d8 into develop Feb 17, 2020
@sam-aws sam-aws deleted the no-updates-here branch February 17, 2020 23:04
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

5 participants