feat(store): promote command #2082
Conversation
To be undone before merging
|
|
||
| name = "promote" | ||
| help_msg = "Promote a charm from one channel to another on Charmhub." | ||
| overview = "TODO" |
There was a problem hiding this comment.
I think few words may be helpful as it'll land in the docs 😄
| overview = "TODO" | |
| overview = "TODO" |
There was a problem hiding this comment.
Will do - I've asked for your second review for other things ITMT
| # 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.
| 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.
| @@ -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
No description provided.