feat(store): promote command #2082
Conversation
To be undone before merging
| # Check snapcraft for equiv logic | ||
| from_channel = charmcraft.store.models.ChannelData.from_str( | ||
| parsed_args.from_channel | ||
| ) | ||
| to_channel = charmcraft.store.models.ChannelData.from_str( | ||
| parsed_args.to_channel | ||
| ) | ||
| if None in (from_channel.track, to_channel.track): | ||
| package_metadata = store.get_package_metadata(name) | ||
| default_track = package_metadata.default_track | ||
| if from_channel.track is None: | ||
| from_channel = dataclasses.replace(from_channel, track=default_track) | ||
| if to_channel.track is None: | ||
| to_channel = dataclasses.replace(to_channel, track=default_track) |
There was a problem hiding this comment.
I'm not sure about this logic, shouldn't we just make it explicit and error out if track is missing ?
There was a problem hiding this comment.
If both are missing, will it try to promote from default to default?
There was a problem hiding this comment.
I don't see why we would want to error out rather than using the default track. An example command that would trigger both of these conditions is:
charmcraft promote --from-channel=candidate --to-channel=stable
In this case, we'll look up the default track (typically latest) and insert it, so the command above is equivalent to:
charmcraft promote --from-channel=latest/candidate --to-channel=latest/stable
if the default track is latest.
There was a problem hiding this comment.
What if there are multiple tracks like this with default set to 22_04_13.2 ?
24_04_13.2
22_04_13.2
20_04_11.1
Are you sure that you know what was the intention of the user ?
I could think that this command may allow promoting if there is a single track, but with multiple available it just seems wrong.
| with emit.pause(): | ||
| print( | ||
| tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
Any particular reason why it can't be a progress as well (beside not logging it) ?
There was a problem hiding this comment.
emit.progress tries to represent it as a single line, even with permanent=True.
There was a problem hiding this comment.
Would it work this way ? It seems to be a bit more consistent and does not leak the sys.stderr implementation detail
| with emit.pause(): | |
| print( | |
| tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | |
| file=sys.stderr, | |
| ) | |
| with emit.open_stream() as stream: | |
| print( | |
| tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | |
| file=stream, | |
| ) |
There was a problem hiding this comment.
open_stream() doesn't result in a file-like object, just an integer with the file pointer, so you'd have to then open that, and it gets ugly.
There was a problem hiding this comment.
You can probably do os.write() then instead of print, but it's up to you.
I think I did emit.progress() previously on multi line output and it worked, that's why I asked in the first place.
| @@ -463,7 +462,7 @@ requires-dist = [ | |||
| { name = "craft-platforms", specifier = "~=0.5" }, | |||
| { name = "craft-providers", specifier = ">=2.0.0" }, | |||
| { name = "craft-providers", specifier = ">=2.1.0" }, | |||
| { name = "craft-store", specifier = ">=3.1.0" }, | |||
| { name = "craft-store", git = "https://github.com/canonical/craft-store" }, | |||
There was a problem hiding this comment.
Reminder to update to the proper release
| with emit.pause(): | ||
| print( | ||
| tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
Would it work this way ? It seems to be a bit more consistent and does not leak the sys.stderr implementation detail
| with emit.pause(): | |
| print( | |
| tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | |
| file=sys.stderr, | |
| ) | |
| with emit.open_stream() as stream: | |
| print( | |
| tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | |
| file=stream, | |
| ) |
| # Check snapcraft for equiv logic | ||
| from_channel = charmcraft.store.models.ChannelData.from_str( | ||
| parsed_args.from_channel | ||
| ) | ||
| to_channel = charmcraft.store.models.ChannelData.from_str( | ||
| parsed_args.to_channel | ||
| ) | ||
| if None in (from_channel.track, to_channel.track): | ||
| package_metadata = store.get_package_metadata(name) | ||
| default_track = package_metadata.default_track | ||
| if from_channel.track is None: | ||
| from_channel = dataclasses.replace(from_channel, track=default_track) | ||
| if to_channel.track is None: | ||
| to_channel = dataclasses.replace(to_channel, track=default_track) |
There was a problem hiding this comment.
If both are missing, will it try to promote from default to default?
| if not parsed_args.yes and not utils.confirm_with_user( | ||
| "Did you mean to promote to a different track? (from " | ||
| f"{from_channel.track} to {to_channel.track})", | ||
| ): | ||
| emit.message("Cancelling.") | ||
| return 64 # Replace with os.EX_USAGE once we drop Windows. |
There was a problem hiding this comment.
I think we shouldn't allow non-interactive promotion between tracks at all
There was a problem hiding this comment.
I don't see why not. As an example, in CI you might want to promote 3/edge to latest/edge
There was a problem hiding this comment.
Going further with your example, you can also promote latest/edge to 3/stable without a confirmation
| their related resources, to another channel. | ||
|
|
||
| The most common use is to promote a charm to a more stable risk value on a | ||
| single track: |
There was a problem hiding this comment.
Does it render itself correctly in both cli and docs ?
| if not parsed_args.yes and not utils.confirm_with_user( | ||
| "Did you mean to promote to a different track? (from " | ||
| f"{from_channel.track} to {to_channel.track})", | ||
| ): | ||
| emit.message("Cancelling.") | ||
| return 64 # Replace with os.EX_USAGE once we drop Windows. |
There was a problem hiding this comment.
Going further with your example, you can also promote latest/edge to 3/stable without a confirmation
| ) | ||
|
|
||
| @override | ||
| def run(self, parsed_args: argparse.Namespace) -> int | None: |
There was a problem hiding this comment.
I've also noticed that this command has plenty of logic, but there is not a single test that covers it.
No description provided.