From 8298a71392055e6512f4bd4cc0f5e9931e4f9437 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Wed, 3 Jul 2024 09:17:28 +0100 Subject: [PATCH 1/2] feat: conventional commits static analysis Use linting to ensure proper commit messages. This will use the default config out of the box, and we can add more if we feel the need. --- .github/workflows/static-analysis.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .github/workflows/static-analysis.yaml diff --git a/.github/workflows/static-analysis.yaml b/.github/workflows/static-analysis.yaml new file mode 100644 index 00000000..b4fb3033 --- /dev/null +++ b/.github/workflows/static-analysis.yaml @@ -0,0 +1,18 @@ +name: "Static Analysis" +on: + push: + branches: [master] + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + +permissions: + contents: read + +jobs: + conventional-commits: + name: Check conventional commits + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: wagoid/commitlint-github-action@v6 + From 956aa5a7188369d0d9528f81598678955c93e0f7 Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Fri, 28 Jun 2024 19:19:02 +0100 Subject: [PATCH 2/2] fix(refresh): bug with revisions We are incorrectly refreshing charms. The current method queries the controller for the charm's current origin and charm url In pylibjuju, we then just overwrite the appropriate fields. However, this leaves behind id_ and hash_ This means when we later refresh, we are always returned the most recent revision by charmhub, no matter what revision we specify. The way we handle this in the Juju CLI is by reconstructing the charm origin using MakeOrigin: https://github.com/juju/juju/blob/a03ade4d358b1b0f135374b15b1bcd6b46d0ba0f/cmd/juju/application/utils/origin.go#L20-L75 This ensures only the fields we expect are present. Replicate this method, ensuring we behave the same as the CLI. Fixes: https://github.com/juju/python-libjuju/issues/1057 --- juju/application.py | 43 ++++++++++++--------- tests/integration/test_application.py | 12 ++++++ tests/unit/test_application.py | 55 ++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/juju/application.py b/juju/application.py index e1e08ef2..3a80f23f 100644 --- a/juju/application.py +++ b/juju/application.py @@ -12,12 +12,13 @@ from .bundle import get_charm_series, is_local_charm from .client import client from .errors import JujuApplicationConfigError, JujuError -from .origin import Channel, Source +from .origin import Channel from .placement import parse as parse_placement from .relation import Relation from .status import derive_status from .url import URL from .utils import block_until +from .version import DEFAULT_ARCHITECTURE log = logging.getLogger(__name__) @@ -691,13 +692,15 @@ async def refresh( if charm_url_origin_result.error is not None: err = charm_url_origin_result.error raise JujuError(f'{err.code} : {err.message}') - origin = charm_url_origin_result.charm_origin + current_origin = charm_url_origin_result.charm_origin if path is not None or (switch is not None and is_local_charm(switch)): - await self.local_refresh(origin, force, force_series, + await self.local_refresh(current_origin, force, force_series, force_units, path or switch, resources) return + origin = _refresh_origin(current_origin, channel, revision) + # If switch is not None at this point, that means it's a switch to a store charm charm_url = switch or charm_url_origin_result.url parsed_url = URL.parse(charm_url) @@ -706,20 +709,6 @@ async def refresh( if parsed_url.schema is None: raise JujuError(f'A ch: or cs: schema is required for application refresh, given : {str(parsed_url)}') - if revision is not None: - origin.revision = revision - - # Make the source-specific changes to the origin/channel/url - # (and also get the resources necessary to deploy the (destination) charm -- for later) - origin.source = Source.CHARM_HUB.value - if channel: - ch = Channel.parse(channel).normalize() - origin.risk = ch.risk - origin.track = ch.track - - charmhub = self.model.charmhub - charm_resources = await charmhub.list_resources(charm_name) - # Resolve the given charm URLs with an optionally specified preferred channel. # Channel provided via CharmOrigin. resolved_charm_with_channel_results = await charms_facade.ResolveCharms( @@ -761,8 +750,7 @@ async def refresh( else: _arg_res_filenames[res] = filename_or_rev - # Already prepped the charm_resources - # Now get the existing resources from the ResourcesFacade + # Get the existing resources from the ResourcesFacade request_data = [client.Entity(self.tag)] resources_facade = client.ResourcesFacade.from_connection(self.connection) response = await resources_facade.ListResources(entities=request_data) @@ -771,6 +759,9 @@ async def refresh( for resource in response.results[0].resources } + charmhub = self.model.charmhub + charm_resources = await charmhub.list_resources(charm_name) + # Compute the difference btw resources needed and the existing resources resources_to_update = [] for resource in charm_resources: @@ -917,6 +908,20 @@ async def get_metrics(self): return await self.model.get_metrics(self.tag) +def _refresh_origin(current_origin: client.CharmOrigin, channel=None, revision=None) -> client.CharmOrigin: + if channel is not None: + channel = Channel.parse(channel).normalize() + + return client.CharmOrigin( + source=current_origin.source, + track=channel.track if channel else current_origin.track, + risk=channel.risk if channel else current_origin.risk, + revision=revision if revision is not None else current_origin.revision, + base=current_origin.base, + architecture=current_origin.get('architecture', DEFAULT_ARCHITECTURE), + ) + + class ExposedEndpoint: """ExposedEndpoint stores the list of CIDRs and space names which should be allowed access to the port ranges that the application has opened for a diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index 0ab2f439..ecac8b7d 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -286,6 +286,18 @@ async def test_local_refresh(): base=client.Base("20.04", "ubuntu")) +@base.bootstrapped +@pytest.mark.asyncio +async def test_refresh_revision(): + async with base.CleanModel() as model: + app = await model.deploy('juju-qa-test', channel="latest/stable", revision=23) + # NOTE: juju-qa-test revision 26 has been released to this channel + await app.refresh(revision=25) + + charm_url = URL.parse(app.data['charm-url']) + assert charm_url.revision == 25 + + @base.bootstrapped @pytest.mark.asyncio async def test_trusted(): diff --git a/tests/unit/test_application.py b/tests/unit/test_application.py index abbf758f..e2b9a588 100644 --- a/tests/unit/test_application.py +++ b/tests/unit/test_application.py @@ -6,8 +6,10 @@ import asyncio from juju.model import Model -from juju.application import (Application, ExposedEndpoint) +from juju.application import Application, ExposedEndpoint, _refresh_origin from juju.errors import JujuError +from juju.client import client +from juju.origin import Source class TestExposeApplication(unittest.IsolatedAsyncioTestCase): @@ -177,3 +179,54 @@ async def test_refresh_mutually_exclusive_kwargs(self, mock_conn): with self.assertRaises(ValueError): await app.refresh(switch="charm1", path="/path/to/charm2") + + def test_refresh_origin(self): + current_origin = client.CharmOrigin( + source=str(Source.CHARM_HUB), + track="latest", + risk="stable", + revision=100, + base=client.Base("24.04", "ubuntu"), + architecture="amd64", + ) + + origin = _refresh_origin(current_origin, None, None) + self.assertEqual(origin, current_origin) + + origin = _refresh_origin(current_origin, None, 101) + self.assertEqual(origin.revision, 101) + # Check source, base & arch do not change + self.assertEqual(origin.source, current_origin.source) + self.assertEqual(origin.base, current_origin.base) + self.assertEqual(origin.architecture, current_origin.architecture) + + origin = _refresh_origin(current_origin, None, 0) + self.assertEqual(origin.revision, 0) + # Check source, base & arch do not change + self.assertEqual(origin.source, current_origin.source) + self.assertEqual(origin.base, current_origin.base) + self.assertEqual(origin.architecture, current_origin.architecture) + + origin = _refresh_origin(current_origin, "12/edge", None) + self.assertEqual(origin.track, "12") + self.assertEqual(origin.risk, "edge") + # Check source, base & arch do not change + self.assertEqual(origin.source, current_origin.source) + self.assertEqual(origin.base, current_origin.base) + self.assertEqual(origin.architecture, current_origin.architecture) + + def test_refresh_origin_drops_id_hash(self): + current_origin = client.CharmOrigin( + source=str(Source.CHARM_HUB), + track="latest", + risk="stable", + revision=100, + base=client.Base("24.04", "ubuntu"), + architecture="amd64", + id_="id", + hash_="hash", + ) + + origin = _refresh_origin(current_origin, None, None) + self.assertIsNone(origin.id_) + self.assertIsNone(origin.hash_)