add missing "revision" parameter to addModel step. #1043

Thanhphan1147 · 2024-04-15T16:22:42Z

Description

When deploying a bundle, although the plan is properly generated with the correct revision for the charms in the bundle, and the change steps are generated with the correct parameter. I believe that the addCharm step does not take into account the specified "revision" of the charm to deploy, as a result the revision deployed is the latest revision instead of the specified revision. ref:

python-libjuju/juju/bundle.py

Lines 709 to 713 in 5ed5ae4

    
           origin = client.CharmOrigin(source=Source.CHARM_HUB.value, 
        
                                       architecture=arch, 
        
                                       risk=ch.risk, 
        
                                       track=ch.track, 
        
                                       base=base)

This PR simply adds the missing parameter to the AddCharmChange step and propagate the value to client.CharmOrigin to add the correct revision.

QA Steps

Create a bundle containing one or more charms with revision
Deploy the bundle with model.deploy
The deployed charms in the bundle should have the revision specified in the bundle.

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
Integration tests, with comments saying what you're testing
~~doc.go added or updated in changed package~~ (I think this is too small of a change to warrant a doc update)

jujubot · 2024-04-15T16:22:45Z

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

jujubot · 2024-04-15T16:22:46Z

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

…charms

jameinel

It definitely feels like we are missing a test of some form (plausibly an integration test), to ensure that bundles with charm revisions are properly deployed, but the change itself seems plausible.

juju/bundle.py

…thub.com:Thanhphan1147/python-libjuju into add_missing_revision_params_in_bundle_add_charms

cderici

Thanks for the change @Thanhphan1147! A little fix for the failing test is needed on this one, let's get that passing, then we can take over and do some manual QA to get this landed👍

tests/integration/test_model.py

Co-authored-by: Caner Derici <caner.derici@canonical.com>

…thub.com:Thanhphan1147/python-libjuju into add_missing_revision_params_in_bundle_add_charms

Thanhphan1147 · 2024-04-17T09:37:03Z

the added integration test seems to be passing locally for me.

Thanhphan1147 · 2024-04-17T21:45:15Z

@cderici Seems like a few other integration tests are having issues, maybe a few updates on the bundle definitions as jammy is not a supported series for the old charms.

The one I added seems to be fine:

[gw0] [ 45%] PASSED tests/integration/test_model.py::test_deploy_bundle_with_pinned_charm_revision

cderici

This LGTM, manual QA performed against 3.4, everything seems to be working fine 👍

@Thanhphan1147 I can confirm that the failing tests are known test failures that'll be fixed soon (and unrelated to this change), this can land safely 👍thanks again for the contribution, much appreciated!

jameinel · 2024-04-18T11:58:18Z

@Thanhphan1147 Caner is off until Monday. I would tend to wait for him to merge this, but if you need it sooner, let us know.

Thanhphan1147 · 2024-04-18T13:47:31Z

@jameinel nothing urgent from me, thanks for your help on this PR!

…charms

cderici · 2024-04-23T00:52:12Z

/merge

cderici · 2024-04-23T01:26:15Z

/build

cderici · 2024-04-23T14:23:45Z

/merge

@Aflynn50

#1056 ## What's Changed * Make consume respect the controller name in the url by @Aflynn50 in #1038 * Fix multiline description in textarea in bug template GH workflow by @cderici in #1041 * Fix issue with microk8s proxy by @Aflynn50 in #1044 * add missing "revision" parameter to addModel step. by @Thanhphan1147 in #1043 * Add user api to pylibjuju documentation by @cderici in #1049 ## New Contributors * @Thanhphan1147 made their first contribution in #1043 **Full Changelog**: 3.4.0.0...3.5.0.0

add missing "revision" parameter to addModel step.

da3287e

Merge branch 'master' into add_missing_revision_params_in_bundle_add_…

84d487a

…charms

jameinel reviewed Apr 15, 2024

View reviewed changes

juju/bundle.py Show resolved Hide resolved

Thanhphan1147 added 9 commits April 16, 2024 16:44

update unit tests for new param

f50d247

Merge branch 'add_missing_revision_params_in_bundle_add_charms' of gi…

46c58b0

…thub.com:Thanhphan1147/python-libjuju into add_missing_revision_params_in_bundle_add_charms

sort import and add integration test

b450bc6

add bundle with pinned revision test file

a649d1f

refactor imports

2c4b8cd

more import refactoring

7dfcfd0

add missing newline

73c216d

remove type-hint

8ae928d

remove extra new line

7748838

Thanhphan1147 requested a review from jameinel April 16, 2024 15:53

cderici suggested changes Apr 17, 2024

View reviewed changes

tests/integration/test_model.py Outdated Show resolved Hide resolved

tests/integration/test_model.py Outdated Show resolved Hide resolved

Thanhphan1147 and others added 2 commits April 17, 2024 11:20

Update tests/integration/test_model.py

fcfa17d

Co-authored-by: Caner Derici <caner.derici@canonical.com>

Update tests/integration/test_model.py

94f03d7

Co-authored-by: Caner Derici <caner.derici@canonical.com>

Thanhphan1147 requested a review from cderici April 17, 2024 09:22

Thanhphan1147 added 2 commits April 17, 2024 11:23

correct typo

cd01ce3

Merge branch 'add_missing_revision_params_in_bundle_add_charms' of gi…

3bee871

…thub.com:Thanhphan1147/python-libjuju into add_missing_revision_params_in_bundle_add_charms

Thanhphan1147 added 2 commits April 17, 2024 12:20

use an old revision to avoid false-positives

bc84a16

remove series as focal is not supported for rev7

c8648b1

cderici approved these changes Apr 17, 2024

View reviewed changes

Merge branch 'master' into add_missing_revision_params_in_bundle_add_…

04f0e53

…charms

jujubot merged commit d4f6677 into juju:master Apr 23, 2024

Aflynn50 mentioned this pull request May 30, 2024

Release notes for 3.5.0.0 #1056

Merged

Thanhphan1147 deleted the add_missing_revision_params_in_bundle_add_charms branch August 16, 2024 13:25

	origin = client.CharmOrigin(source=Source.CHARM_HUB.value,
	architecture=arch,
	risk=ch.risk,
	track=ch.track,
	base=base)

add missing "revision" parameter to addModel step. #1043

add missing "revision" parameter to addModel step. #1043

Uh oh!

Conversation

Thanhphan1147 commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

QA Steps

Uh oh!

jujubot commented Apr 15, 2024

Uh oh!

jujubot commented Apr 15, 2024

Uh oh!

jameinel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cderici left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Thanhphan1147 commented Apr 17, 2024

Uh oh!

Thanhphan1147 commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cderici left a comment

Choose a reason for hiding this comment

Uh oh!

jameinel commented Apr 18, 2024

Uh oh!

Thanhphan1147 commented Apr 18, 2024

Uh oh!

cderici commented Apr 23, 2024

Uh oh!

cderici commented Apr 23, 2024

Uh oh!

cderici commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Thanhphan1147 commented Apr 15, 2024 •

edited

Loading

Thanhphan1147 commented Apr 17, 2024 •

edited

Loading