fix(refresh): bug with revisions #1067
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jack-w-shaw
force-pushed
the
JUJU-6262_fix_refresh_revision
branch
from
4d56e56
to
d44b2a1
Compare
jack-w-shaw
changed the title
jack-w-shaw
force-pushed
the
JUJU-6262_fix_refresh_revision
branch
2 times, most recently
from
5482461
to
890ba55
Compare
cderici
suggested changes
Jul 3, 2024
juju/application.py
Outdated
|
|
||
| charmhub = self.model.charmhub | ||
| charm_resources = await charmhub.list_resources(charm_name) | ||
| origin = client.CharmOrigin( |
There was a problem hiding this comment.
let's move this out into a helper function and have some unit tests against this to protect the logic for any regressions.
There was a problem hiding this comment.
What logic exactly are you referring to here?
There was a problem hiding this comment.
More like decisions I meant to say, we make a bunch of decisions here, it would be nice if we have unit tests that assert those decisions so if any of them are violated in the future we'd catch it before it breaks.
source=str(Source.CHARM_HUB),
risk=channel.risk if channel else current_origin.risk,
track=channel.track if channel else current_origin.track,
revision=revision or current_origin.revision,
base=current_origin.base,
architecture=current_origin.get('architecture', DEFAULT_ARCHITECTURE),
56b16bc
to
7ae2a4d
Compare
cderici
approved these changes
Jul 9, 2024
jack-w-shaw
force-pushed
the
JUJU-6262_fix_refresh_revision
branch
from
643231b
to
7ae2a4d
Compare
|
/merge |
1 similar comment
|
/merge |
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: juju#1057
jack-w-shaw
force-pushed
the
JUJU-6262_fix_refresh_revision
branch
from
7ae2a4d
to
956aa5a
Compare
|
/merge |
jujubot
added a commit
that referenced
this pull request
Jul 11, 2024
#1071 ## What's Changed * fix parsing of storage constraints by @luissimas in #1053 * Add setuptools to tox.ini by @Aflynn50 in #1058 * fix(refresh): bug with revisions by @jack-w-shaw in #1067 * feat: conventional commits static analysis by @SimonRichardson in #1068 * fix(series): add noble support by @jack-w-shaw in #1063 * fix zones constrains list parsing by @luissimas in #1054 * fix(model): fix wrong instanciation of list-secrets facade by @gboutry in #1065 * fix(makefile): run .tox before lint in makefile target by @cderici in #1069
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
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.
Do something similar in pylibjuju, ensuring we behave the same as the CLI.
Fixes: #1057
QA Steps
Unit tests
Integration tests
Manual tests