-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat(store): promote command #2082
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -25,13 +25,14 @@ | |||||||||||||||||||||
import re | ||||||||||||||||||||||
import shutil | ||||||||||||||||||||||
import string | ||||||||||||||||||||||
import sys | ||||||||||||||||||||||
import tempfile | ||||||||||||||||||||||
import textwrap | ||||||||||||||||||||||
import typing | ||||||||||||||||||||||
import zipfile | ||||||||||||||||||||||
from collections.abc import Collection | ||||||||||||||||||||||
from operator import attrgetter | ||||||||||||||||||||||
from typing import TYPE_CHECKING, Any | ||||||||||||||||||||||
from typing import TYPE_CHECKING, Any, cast | ||||||||||||||||||||||
|
||||||||||||||||||||||
import yaml | ||||||||||||||||||||||
from craft_application import util | ||||||||||||||||||||||
|
@@ -43,11 +44,13 @@ | |||||||||||||||||||||
from craft_store.models import ResponseCharmResourceBase | ||||||||||||||||||||||
from humanize import naturalsize | ||||||||||||||||||||||
from tabulate import tabulate | ||||||||||||||||||||||
from typing_extensions import override | ||||||||||||||||||||||
|
||||||||||||||||||||||
import charmcraft.store.models | ||||||||||||||||||||||
from charmcraft import const, env, errors, parts, utils | ||||||||||||||||||||||
from charmcraft.application.commands.base import CharmcraftCommand | ||||||||||||||||||||||
from charmcraft.models import project | ||||||||||||||||||||||
from charmcraft.services.store import StoreService | ||||||||||||||||||||||
from charmcraft.store import Store | ||||||||||||||||||||||
from charmcraft.store.models import Entity | ||||||||||||||||||||||
from charmcraft.utils import cli | ||||||||||||||||||||||
|
@@ -830,6 +833,152 @@ def run(self, parsed_args): | |||||||||||||||||||||
emit.message(msg.format(*args)) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
class PromoteCommand(CharmcraftCommand): | ||||||||||||||||||||||
"""Promote a charm in the Store.""" | ||||||||||||||||||||||
|
||||||||||||||||||||||
name = "promote" | ||||||||||||||||||||||
help_msg = "Promote a charm from one channel to another on Charmhub." | ||||||||||||||||||||||
overview = textwrap.dedent( | ||||||||||||||||||||||
"""Promote a charm from one channel to another on Charmhub. | ||||||||||||||||||||||
|
||||||||||||||||||||||
Promotes the current revisions of a charm in a specific channel, as well as | ||||||||||||||||||||||
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: | ||||||||||||||||||||||
|
||||||||||||||||||||||
charmcraft promote --from-channel=candidate --to-channel=stable | ||||||||||||||||||||||
""" | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
@override | ||||||||||||||||||||||
def needs_project(self, parsed_args: argparse.Namespace) -> bool: | ||||||||||||||||||||||
if parsed_args.name is None: | ||||||||||||||||||||||
emit.progress("Inferring name from project file.", permanent=True) | ||||||||||||||||||||||
return True | ||||||||||||||||||||||
return False | ||||||||||||||||||||||
|
||||||||||||||||||||||
@override | ||||||||||||||||||||||
def fill_parser(self, parser: "ArgumentParser") -> None: | ||||||||||||||||||||||
parser.add_argument( | ||||||||||||||||||||||
"--name", | ||||||||||||||||||||||
help="the name of the charm to promote. If not specified, the name will be inferred from the charm in the current directory.", | ||||||||||||||||||||||
) | ||||||||||||||||||||||
parser.add_argument( | ||||||||||||||||||||||
"--from-channel", | ||||||||||||||||||||||
metavar="from-channel", | ||||||||||||||||||||||
help="the channel to promote from", | ||||||||||||||||||||||
required=True, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
parser.add_argument( | ||||||||||||||||||||||
"--to-channel", | ||||||||||||||||||||||
metavar="to-channel", | ||||||||||||||||||||||
help="the channel to promote to", | ||||||||||||||||||||||
required=True, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
parser.add_argument( | ||||||||||||||||||||||
"--yes", | ||||||||||||||||||||||
default=False, | ||||||||||||||||||||||
action="store_true", | ||||||||||||||||||||||
help="Answer yes to all questions.", | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
@override | ||||||||||||||||||||||
def run(self, parsed_args: argparse.Namespace) -> int | None: | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've also noticed that this command has plenty of logic, but there is not a single test that covers it. |
||||||||||||||||||||||
emit.progress( | ||||||||||||||||||||||
f"{self._app.name} {self.name} does not have a stable CLI interface. " | ||||||||||||||||||||||
"Use with caution in scripts.", | ||||||||||||||||||||||
permanent=True, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
store = cast(StoreService, self._services.get("store")) | ||||||||||||||||||||||
|
||||||||||||||||||||||
name = parsed_args.name or self._services.project.name | ||||||||||||||||||||||
|
||||||||||||||||||||||
# 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) | ||||||||||||||||||||||
Comment on lines
+897
to
+910
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If both are missing, will it try to promote from default to default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
In this case, we'll look up the default track (typically
if the default track is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if there are multiple tracks like this with default set to
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. |
||||||||||||||||||||||
|
||||||||||||||||||||||
if to_channel == from_channel: | ||||||||||||||||||||||
raise CraftError( | ||||||||||||||||||||||
"Cannot promote from a channel to the same channel.", | ||||||||||||||||||||||
retcode=64, # Replace with os.EX_USAGE once we drop Windows. | ||||||||||||||||||||||
) | ||||||||||||||||||||||
if to_channel.risk > from_channel.risk: | ||||||||||||||||||||||
dariuszd21 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
command_parts = [ | ||||||||||||||||||||||
self._app.name, | ||||||||||||||||||||||
f"--from-channel={to_channel.name}", | ||||||||||||||||||||||
f"--to-channel={from_channel.name}", | ||||||||||||||||||||||
self.name, | ||||||||||||||||||||||
] | ||||||||||||||||||||||
command = " ".join(command_parts) | ||||||||||||||||||||||
raise CraftError( | ||||||||||||||||||||||
f"Target channel ({to_channel.name}) must be lower risk " | ||||||||||||||||||||||
f"than the source channel ({from_channel.name}).", | ||||||||||||||||||||||
resolution=f"Did you mean: {command}", | ||||||||||||||||||||||
) | ||||||||||||||||||||||
if to_channel.track != from_channel.track: | ||||||||||||||||||||||
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. | ||||||||||||||||||||||
Comment on lines
+931
to
+936
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we shouldn't allow non-interactive promotion between tracks at all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why not. As an example, in CI you might want to promote There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going further with your example, you can also promote |
||||||||||||||||||||||
|
||||||||||||||||||||||
candidates = store.get_revisions_on_channel(name, from_channel.name) | ||||||||||||||||||||||
|
||||||||||||||||||||||
def get_base_strings(bases): | ||||||||||||||||||||||
if bases is None: | ||||||||||||||||||||||
return "" | ||||||||||||||||||||||
return ",".join( | ||||||||||||||||||||||
f"{base.name}@{base.channel}:{base.architecture}" for base in bases | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
presentable_candidates = [ | ||||||||||||||||||||||
{ | ||||||||||||||||||||||
"Revision": info["revision"], | ||||||||||||||||||||||
"Platforms": get_base_strings(info["bases"]), | ||||||||||||||||||||||
"Resource revisions": ", ".join( | ||||||||||||||||||||||
f"{res['name']}: {res['revision']}" for res in info["resources"] | ||||||||||||||||||||||
), | ||||||||||||||||||||||
} | ||||||||||||||||||||||
for info in sorted(candidates, key=lambda x: x["revision"]) | ||||||||||||||||||||||
] | ||||||||||||||||||||||
emit.progress( | ||||||||||||||||||||||
f"The following revisions are on the {from_channel.name} channel:", | ||||||||||||||||||||||
permanent=True, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
with emit.pause(): | ||||||||||||||||||||||
print( | ||||||||||||||||||||||
tabulate(presentable_candidates, tablefmt="plain", headers="keys"), | ||||||||||||||||||||||
file=sys.stderr, | ||||||||||||||||||||||
) | ||||||||||||||||||||||
Comment on lines
+961
to
+965
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason why it can't be a progress as well (beside not logging it) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it work this way ? It seems to be a bit more consistent and does not leak the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can probably do I think I did |
||||||||||||||||||||||
if not parsed_args.yes and not utils.confirm_with_user( | ||||||||||||||||||||||
f"Do you want to promote these revisions to the {to_channel.name} channel?" | ||||||||||||||||||||||
): | ||||||||||||||||||||||
emit.message("Channel promotion cancelled.") | ||||||||||||||||||||||
return 1 | ||||||||||||||||||||||
lengau marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
|
||||||||||||||||||||||
promotion_results = store.release_promotion_candidates( | ||||||||||||||||||||||
name, to_channel.name, candidates | ||||||||||||||||||||||
) | ||||||||||||||||||||||
|
||||||||||||||||||||||
emit.message( | ||||||||||||||||||||||
f"{len(promotion_results)} revisions promoted from {from_channel.name} to {to_channel.name}" | ||||||||||||||||||||||
) | ||||||||||||||||||||||
return 0 | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
||||||||||||||||||||||
class PromoteBundleCommand(CharmcraftCommand): | ||||||||||||||||||||||
"""Promote a bundle in the Store.""" | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -2098,7 +2247,6 @@ def run(self, parsed_args: argparse.Namespace) -> int: | |||||||||||||||||||||
resolution="Pass a valid container transport string.", | ||||||||||||||||||||||
) | ||||||||||||||||||||||
emit.debug(f"Using source path {source_path!r}") | ||||||||||||||||||||||
|
||||||||||||||||||||||
emit.progress("Inspecting source image") | ||||||||||||||||||||||
image_metadata = image_service.inspect(source_path) | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it render itself correctly in both
cli
and docs ?