Plugin update badger #1577

itowlson · 2023-06-12T22:43:22Z

Fixes #1543.

Signed-off-by: itowlson <[email protected]>

lann

A couple of minor nits but I'm also having trouble understanding some of the logic. A few inline comments would help.

crates/plugins/src/badger/mod.rs

crates/plugins/src/badger/store.rs

lann · 2023-06-13T13:22:52Z

crates/plugins/src/badger/store.rs

+ let base_dir = dirs::cache_dir()
+ .or_else(|| dirs::home_dir().map(|p| p.join(".spin")))
+ .ok_or_else(|| anyhow!("Unable to get local data directory or home directory"))?;


It feels like there is some logic here that ought to be extracted into spin-common.

The only thing in cache/spin other than the notifications record seems to be the OCI registry cache... I can move this into common if we want but I'm reluctant to mess with the OCI cache code (especially as part of this PR). Subsequent refactoring maybe?

I think this should reuse default_data_dir from data_dir.rs. Otherwise it will also mess up any colocation of data for package managers

~~@kate-goldenring note this is under dirs::cache_dir, not data_dir.~~ I see this was addressed in the top-level comments.

@itowlson I had this in the back of my mind when I suggested spin_common::spin_dirs over in #1584 (comment) 🤷 Either way I'm fine with punting for now.

crates/plugins/src/badger/store.rs

src/commands/external.rs

itowlson · 2023-06-13T20:56:06Z

Thanks for the feedback - I added a bunch of comments that hopefully clarify things, but of course please let me know if it's still cryptic.

kate-goldenring

The implementation is looking great. Just a few clarifying questions

kate-goldenring · 2023-06-13T18:19:53Z

src/commands/external.rs

@@ -68,6 +75,7 @@ pub async fn execute_external_subcommand(
 }
 plugin_installer.run().await?;
 }
+ None // No update badgering needed if we just installed it!


Suggested change

None // No update badgering needed if we just installed it!

None // No update badgering needed if we just updated and installed it!

kate-goldenring · 2023-06-13T18:40:37Z

src/commands/external.rs

+ terminal::info!("This plugin can be upgraded.", "Version {to} is available,");
+ eprintln!("but may not be backward compatible with your current plugin.");
+ eprintln!("To upgrade, run `{}`.", to.upgrade_command());


Curious. What is the reason for the combination of logging and eprintln? Is it for color?

Logging is used for errors, and eprintln/terminal for user output. We don't want to display errors by default (this is meant to be a silent, unintrusive process), so we log those at info. terminal is used when we want colour highlighting, eprintln for continuation lines that don't want additional colouring.

#1577 (comment)

kate-goldenring · 2023-06-13T20:59:10Z

crates/plugins/src/badger/mod.rs

+//
+// The reason for the Precomputed/Deferred dance is to handle the two cases of:
+// 1. There's no point waiting and doing the calculations because we _know_ we have a decision (or an error).
+// 2. There's a point to waiting because there _might_ be an upgrade, so we want to give the backgroeund


Suggested change

// 2. There's a point to waiting because there _might_ be an upgrade, so we want to give the backgroeund

// 2. There's a point to waiting because there _might_ be an upgrade, so we want to give the background

kate-goldenring · 2023-06-13T21:21:00Z

crates/plugins/src/badger/mod.rs

+ let (eligible_manifests, questionable_manifests) = if self.current_version.major == 0 {
+ (vec![], considerable_manifests)
+ } else {


Given that none of our plugins are v1.0, this badger will always throw "but may not be backward compatible with your current plugin", even if updating a patch version. For example, if i run spin cloud --help with cloud 0.1.0 installed, I get:

This plugin can be upgraded. Version 0.1.1 is available, but may not be backward compatible with your current plugin.

According to semver, it should be backwards compat

I thought semver had an exception for major version 0? If it doesn't, we've been doing it wrong... grin

Indeed you are right: "
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable."

I guess my point is, are we concerned with every badger saying "may not be compatible". I guess this is a given, but i do think for our plugins it would be worth visiting what it would take to see them to 1.0

Yes, and this perhaps comes back to the discussion we had about stability vs maturity - it would be great to try to get our plugins to 1.x even if they are not fully mature, so that we can at least send meaningful signals about interface stability and compatibility. But the risk then is we have to do too many major revs in the early months...

kate-goldenring · 2023-06-13T21:25:30Z

crates/plugins/src/badger/mod.rs

+ let highest_questionable_manifest = questionable_manifests
+ .into_iter()
+ .max_by_key(|pv| pv.version.clone());
+


According to the definition of Both being that the questionable is higher than the elgible, do we need a check here of whether highest_questionable_manifest > highest_eligible_manifest and if false set highest_questionable_manifest to None?

considerable_manifests contains only manifests whose version is >= current version. If HEV exists then its major version is == current major version. If HQV exists, its major version is != current major version. But since HQV major must be >= current major version, this means HQV major > current major. And since current major == HEV major, if both exist then HQV major > HEV major.

I can add a comment explaining this if it would be useful.

just cackled 🤣 thank you for laying all this out! I don't think we need to comment it

kate-goldenring · 2023-06-13T21:28:08Z

crates/plugins/src/badger/store.rs

+ let base_dir = dirs::cache_dir()
+ .or_else(|| dirs::home_dir().map(|p| p.join(".spin")))
+ .ok_or_else(|| anyhow!("Unable to get local data directory or home directory"))?;


I think this should reuse default_data_dir from data_dir.rs. Otherwise it will also mess up any colocation of data for package managers

kate-goldenring · 2023-06-13T21:31:23Z

crates/plugins/src/badger/store.rs

+ // There is a potential race condition here if someone runs two plugins at
+ // the same time. As this is unlikely, and the worst outcome is that a user
+ // misses a badger or gets a double badger, let's not worry about it for now.


won't the PR to prevent simultaneous updates help with this too?

Sadly, no - the potential race here is the load-save cycle for the notification records themselves.

crates/plugins/src/badger/store.rs

itowlson · 2023-06-13T21:56:58Z

@kate-goldenring for some reason I can't reply to the comment on lines 44-46

I think this should reuse default_data_dir from data_dir.rs. Otherwise it will also mess up any colocation of data for package managers

I chose cache_dir because this is "does not matter if the user blows it away" material - it's purely advisory. But yeah it did feel a bit odd having it in a different place from the plugins. Open to opinion.

kate-goldenring · 2023-06-13T22:25:02Z

@kate-goldenring for some reason I can't reply to the comment on lines 44-46

I think this should reuse default_data_dir from data_dir.rs. Otherwise it will also mess up any colocation of data for package managers

I chose cache_dir because this is "does not matter if the user blows it away" material - it's purely advisory. But yeah it did feel a bit odd having it in a different place from the plugins. Open to opinion.

@itowlson that seems like a reasonable distinction to me. I am just thinking of when people uninstall Spin we have multiple directories that need cleanup but registry uses it anyways so SGTM

lann · 2023-06-14T14:13:57Z

crates/terminal/src/lib.rs

+#[macro_export]
+macro_rules! info {
+ ($highlight:expr, $($arg:tt)*) => {{
+ $crate::ceprint!($crate::colors::bold_cyan(), $highlight);


Not sure how I feel about step! going to stdout while error! and info! go to stderr. For text processing tools the distinction is clear to me: stdout is for "data" that you might be piping to another command, while stderr is for stuff that is only for human consumption. It isn't obvious how/if that distinction applies here.

I sent the update badger to stderr because I explicitly didn't want it mixed up with plugin output that might be getting consumed by a program (e.g. a plugin that outputs JSON). I can be more explicit and call this einfo! or something if you prefer - that might also make it clear the follow-up lines should use eprintln not println. Let me know if you want me to make that or a similar change.

Ah yes that makes perfect sense for 🦡. einfo would make sense to me, especially to help differentiate from step.

lann · 2023-06-14T14:15:45Z

crates/plugins/src/badger/mod.rs

+
+const BADGER_TIMEOUT_DAYS: i64 = 14;
+
+// How the checker works:


Thanks, very helpful!

Signed-off-by: itowlson <[email protected]>

itowlson · 2023-06-14T21:43:42Z

@lann added a comment on the error swallowing, and changed to terminal::einfo!. I think that covers all your remaining comments - do you want to re-review, or are you happy for me to merge on that basis?

lann · 2023-06-14T21:45:04Z

Merge away!

Plugin update badger

729723d

Signed-off-by: itowlson <[email protected]>

itowlson requested review from lann, kate-goldenring and karthik2804 June 12, 2023 22:43

lann reviewed Jun 13, 2023

View reviewed changes

melissaklein24 added this to the 1.4.0 milestone Jun 13, 2023

kate-goldenring reviewed Jun 13, 2023

View reviewed changes

itowlson force-pushed the plugins-badger-background-process-eek-eek-eek branch from c369712 to a769322 Compare June 13, 2023 22:22

kate-goldenring approved these changes Jun 13, 2023

View reviewed changes

lann reviewed Jun 14, 2023

View reviewed changes

Explain what is going on

1afc07c

Signed-off-by: itowlson <[email protected]>

itowlson force-pushed the plugins-badger-background-process-eek-eek-eek branch from a769322 to 1afc07c Compare June 14, 2023 21:41

itowlson merged commit 44d7d5d into fermyon:main Jun 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin update badger #1577

Plugin update badger #1577

itowlson commented Jun 12, 2023

lann left a comment

lann Jun 13, 2023

itowlson Jun 13, 2023

kate-goldenring Jun 13, 2023

lann Jun 14, 2023 •

edited

Loading

itowlson commented Jun 13, 2023

kate-goldenring left a comment

kate-goldenring Jun 13, 2023

kate-goldenring Jun 13, 2023

itowlson Jun 13, 2023

lann Jun 14, 2023

kate-goldenring Jun 13, 2023

kate-goldenring Jun 13, 2023

itowlson Jun 13, 2023

kate-goldenring Jun 13, 2023

itowlson Jun 13, 2023

kate-goldenring Jun 13, 2023 •

edited

Loading

itowlson Jun 13, 2023

kate-goldenring Jun 13, 2023

kate-goldenring Jun 13, 2023

kate-goldenring Jun 13, 2023

itowlson Jun 13, 2023

itowlson commented Jun 13, 2023

kate-goldenring commented Jun 13, 2023

lann Jun 14, 2023

itowlson Jun 14, 2023

lann Jun 14, 2023

lann Jun 14, 2023

itowlson commented Jun 14, 2023

lann commented Jun 14, 2023

	None // No update badgering needed if we just installed it!
	None // No update badgering needed if we just updated and installed it!

	// 2. There's a point to waiting because there _might_ be an upgrade, so we want to give the backgroeund
	// 2. There's a point to waiting because there _might_ be an upgrade, so we want to give the background


		const BADGER_TIMEOUT_DAYS: i64 = 14;

		// How the checker works:

Plugin update badger #1577

Plugin update badger #1577

Conversation

itowlson commented Jun 12, 2023

lann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lann Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

itowlson commented Jun 13, 2023

kate-goldenring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kate-goldenring Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson commented Jun 13, 2023

kate-goldenring commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson commented Jun 14, 2023

lann commented Jun 14, 2023

lann Jun 14, 2023 •

edited

Loading

kate-goldenring Jun 13, 2023 •

edited

Loading