Skip to content
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

[JUJU-1823] Bug 1990182 refresh cs to ch #14628

Merged
merged 6 commits into from Sep 23, 2022

Conversation

hmlanigan
Copy link
Member

The charm origin for charmstore charms does not contain an os, but does contain a series. This creates a problem when refreshing an application using the switch flag to go from a charmstore version of the charm to a charmhub version of the charm. The refresh request made to charmhub is containing an Series while missing the OS and fails.

No upgrade step is necessary due to the validation and fixing done by these changes in this PR. If a series is correct, juju can always determine an OS for it. Do so if DeduceOrigin or ResolveCharm only have a series in the arguments. This is helps protect against bad requests from non juju clients as well when ResolveCharm is called.

Drive by change to not require mongo in testing when it's not used included

QA steps

$ juju bootstrap --bootstrap-series bionic localhost 
$ juju deploy cs:aodh-56 csaodh-bionic --series bionic
Located charm "aodh" in charm-store, revision 56
Deploying "csaodh-bionic" from charm-store charm "aodh", revision 56 in channel stable on bionic
$ juju refresh csaodh-bionic --switch ch:aodh --channel latest/stable
Added charm-hub charm "aodh", revision 57 in channel latest/stable, to the model
Leaving endpoints in "alpha": admin, amqp, certificates, cluster, ha, identity-service, internal, mongodb, nrpe-external-master, public, shared-db

Bug reference

This PR fixes part of the bug. To fix the remainer, an apt update, apt upgrade of the controller machines is required. The /usr/share/distro-info/ubuntu.csv file is out of date. Jujud on the controller(s) must be restarted after the file is updated.

https://bugs.launchpad.net/juju/+bug/1990182

In case an OS for the Series is not provided in the Platform, determine
the OS if a series is specified. Helps to ensure that charmhub requests
are valid.
In case an OS for the Series is not provided in the Platform, determine
the OS if a series is specified. Helps to ensure that charmhub requests
are valid.

Validate the origin has a platform.
Copy link
Member

@cderici cderici left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from bundles complaining, this LGTM.

core/charm/repository/charmhub.go Outdated Show resolved Hide resolved
@hpidcock hpidcock added 2.9 and removed 2.9 labels Sep 20, 2022
@hmlanigan hmlanigan added the bug The PR addresses a bug label Sep 20, 2022
Represent changes to DeduceOrigin and Resolve charm where an OS is added
to the origin if a series is present.
@hmlanigan
Copy link
Member Author

@benhoyt, @cderici I'll add integration tests for juju refresh including --switch in a new PR

Copy link
Member

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, assuming integration tests come in a follow-up PR. It's a pity we need to do the GetOSFromSeries dance, but oh well. I didn't get a chance to run QA steps today, but will tomorrow morning.

core/charm/repository/charmhub.go Show resolved Hide resolved
core/charm/repository/charmhub.go Outdated Show resolved Hide resolved
@hmlanigan
Copy link
Member Author

/merge

2 similar comments
@hmlanigan
Copy link
Member Author

/merge

@jameinel
Copy link
Member

/merge

@jujubot jujubot merged commit ef6b7a7 into juju:2.9 Sep 23, 2022
@wallyworld wallyworld mentioned this pull request Sep 26, 2022
3 tasks
jujubot added a commit that referenced this pull request Sep 26, 2022
#14661

Merge 2.9

#14637
#14638
#14641
#14612
#14646
#14647
#14649
#14628
#14654

Conflicts
```
# Conflicts:
# apiserver/facades/client/application/deploy.go
# apiserver/facades/client/charmhub/charmhub_test.go
# apiserver/facades/client/charmhub/convert.go
# apiserver/facades/client/charms/conversions.go
# apiserver/facades/client/machinemanager/machinemanager.go
# apiserver/facades/client/machinemanager/machinemanager_test.go
# apiserver/facades/client/machinemanager/register.go
# apiserver/facades/client/resources/repository.go
# apiserver/facades/schema.json
# cmd/juju/application/deploy_test.go
# cmd/juju/application/deployer/bundlehandler.go
# cmd/juju/machine/add.go
# core/charm/strategies.go
# core/charm/strategies_test.go
# core/series/supportedseries.go
# core/series/supportedseries_test.go
# environs/manual/winrmprovisioner/provisioner_test.go
# rpc/params/applications.go
# rpc/params/params.go
# state/charm.go
# tests/includes/storage.sh
```

## Checklist

- [X] Code style: imports ordered, good names, simple structure, etc
- [X] Comments saying why design decisions were made
- [X] Go unit tests, with comments saying what you're testing
- ~[ ] [Integration tests](https://github.com/juju/juju/tree/develop/tests), with comments saying what you're testing~
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

See PRs
@hmlanigan hmlanigan deleted the bug-1990182-refresh-cs-to-ch branch September 26, 2022 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 bug The PR addresses a bug
Projects
None yet
6 participants