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.

Sign up for GitHub

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

Jump to bottom

[JUJU-1019] Bug 1969929 bundle revision only #13996

Merged
merged 9 commits into from May 11, 2022
Merged

[JUJU-1019] Bug 1969929 bundle revision only #13996

merged 9 commits into from May 11, 2022

Conversation

hmlanigan
Copy link
Member

Requires juju/charm#393

Deploying a charm by revision also requires a channel to indicate where to upgrade from in the future. Revisions can be in multiple channels, therefore we cannot guess which one the user intends.

Deploying a bundle by revision requires only the revision. The revision may be in multiple channels, however juju has no concept of a bundle once deployed.

Move the check of the revision and channel flags use into the deployer factory to allow for the different behaviors. This is where juju determines if what is being deployed is a charm or a bundle. For a bundle, if ResolveBundleURL returns an IsNotValid error, the deployer factory knows the entity is not a bundle and moves on.

There was a regression in behavior as the apiserver and params were no longer translating and restoring NotValid errors. Fixed this, and ensured that ResolveWithPreferredChannel errors would be restored with valid types in the client charm adapter code. ResolveWithPreferredChannel is in the core package, we should not import the apiserver code there. There may be more places RestoreError is required.

QA steps

$ juju deploy deploy charmed-kubernetes --revision=733 --dry-run
Located bundle "charmed-kubernetes" in charm-hub, revision 733
...
Deploy of bundle completed.

$ juju deploy ubuntu --revision 18
ERROR specifying a revision requires a channel for future upgrades. Please use --channel

$ juju deploy ubuntu --revision 18 --channel stable
Located charm "ubuntu" in charm-hub, revision 18
Deploying "ubuntu" from charm-hub charm "ubuntu", revision 18 in channel stable on focal

# a charmstore charm does not require a channel
$ juju deploy cs:ubuntu --revision 18 csubuntu
Located charm "ubuntu" in charm-store, revision 18
Deploying "csubuntu" from charm-store charm "ubuntu", revision 18 in channel stable on focal

Bug reference

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

@hmlanigan hmlanigan added bug The PR addresses a bug do not merge Even if a PR has been approved, do not merge the PR! 2.9 labels May 2, 2022
hmlanigan and others added 7 commits May 10, 2022 09:48
@hmlanigan
Add translation of NotValid errors across the wire.
0e45dc0
@hmlanigan
Restore error types after results come across the wire. Allows for type
fecf894 
checking further down stream.
@hmlanigan
Moving check on revision and channel flags.
d10c156 
Related to LP1969929. There are different checks based on charm or
bundle. However it's unknown which is deploying until the Deployer.
Moving check.
@hmlanigan
Restore apiserver errors.
288323c 
The call is done inside of core, so it cannot be done there.
@hmlanigan
Avoid 'not valid not valid', use NewNotValid instead of NotValidf
eecc41e
@hmlanigan
Add Code to error messages in test.
7e90fe3 
Needed to pass tests after NotValid errors added to apiserver errors for
translating the type over the wire.
@hmlanigan @jameinel
Fix misspelling in verified.
0490775 
Co-authored-by: John Arbash Meinel <john@arbash-meinel.com>
@hmlanigan hmlanigan force-pushed the bug-1969929-bundle-revision-only branch from a57b6f2 to a568735 Compare May 10, 2022 13:49
@hmlanigan hmlanigan removed the do not merge Even if a PR has been approved, do not merge the PR! label May 10, 2022
juanmanuel-tirado
juanmanuel-tirado approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@juanmanuel-tirado juanmanuel-tirado left a comment

Choose a reason for hiding this comment

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

LG2M

QA passed

@hmlanigan hmlanigan force-pushed the bug-1969929-bundle-revision-only branch from a568735 to 9a33ad5 Compare May 10, 2022 17:10
@hmlanigan
Add Code to error messages in test.
cda7fa0 
Needed to pass tests after NotValid errors added to apiserver errors for
translating the type over the wire.
@hmlanigan
Copy link
Member Author

$$merge$$

@jujubot jujubot merged commit 36865de into juju:2.9 May 11, 2022
@wallyworld wallyworld mentioned this pull request May 19, 2022
Merged
jujubot added a commit that referenced this pull request May 19, 2022
@jujubot
Merge pull request #14050 from wallyworld/merge-2.9-20220519
2fd10f4 
#14050

Merge 2.9, but revert the commit which deleted secrets from 2.9

#13996 [JUJU-1019] Bug 1969929 bundle revision only
#14019 [JUJU-1082] Fix inst filtering to avoid arch mismatches
#14020 [JUJU-1069] CI improvements
#14021 [JUJU-1079] Update volumes if statefulset spec changed;
#13684 [JUJU-544] Remove redundant ifCredentialValid wrappers from model manifolds
#14002 [JUJU-1054] Ensure to convert pod status to juju status consistent across operators
#14024 Juju 1061 add machine private key
#14026 [JUJU-1077] Refactor unit tests
#14033 [JUJU-1103] Add --cert option to microk8s refresh-cert
#13546 [JUJU-299] Store unit CharmURL as a string reference
#14025 [JUJU-1089] Deprecated note for --no-download flag in create-backup
#14023 [JUJU-1070] Fix/lp 1971560
#14027 Fix K8s application removal in pre-initialized error state
#14030 [JUJU-1099] Restore "Store unit CharmURL as a string reference"
#14037 [JUJU-1109] Fix encoding for interfaceAddressDisplay, used by the network-get hook tool
#14028 [JUJU-1091] Cloud-init wait for IP
#14029 [JUJU-1070] Use first 6 digits for short model UUID;
#14034 [JUJU-1070] Use first 6 digits for short model UUID;
#14042 Use default arch when provisioning a machine
#14045 Update references to jujucharms.com
#14046 Address consistency in use of id/Id/ID in command line output
#14047 Adjust the default log level for installing/starting a service

```
# Conflicts:
# apiserver/facades/controller/firewaller/firewaller.go
# apiserver/facades/controller/firewaller/firewaller_test.go
# apiserver/facades/controller/firewaller/firewaller_unit_test.go
# apiserver/facades/controller/remoterelations/mock_test.go
# apiserver/facades/controller/remoterelations/remoterelations_test.go
# caas/kubernetes/provider/application/application_test.go
# caas/kubernetes/provider/k8s.go
# go.mod
# go.sum
# mongo/mongo_test.go
# rpc/params/apierror.go
# snap/snapcraft.yaml
# version/version.go
# worker/uniter/operation/interface.go
# worker/uniter/operation/runhook_test.go
# worker/uniter/secrets/rotatesecrets.go
# worker/uniter/secrets/rotatesecrets_test.go
```

## QA steps

See PRs

[JUJU-1019]: https://warthogs.atlassian.net/browse/JUJU-1019?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1082]: https://warthogs.atlassian.net/browse/JUJU-1082?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1069]: https://warthogs.atlassian.net/browse/JUJU-1069?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1079]: https://warthogs.atlassian.net/browse/JUJU-1079?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-544]: https://warthogs.atlassian.net/browse/JUJU-544?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1054]: https://warthogs.atlassian.net/browse/JUJU-1054?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1077]: https://warthogs.atlassian.net/browse/JUJU-1077?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1103]: https://warthogs.atlassian.net/browse/JUJU-1103?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-299]: https://warthogs.atlassian.net/browse/JUJU-299?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1089]: https://warthogs.atlassian.net/browse/JUJU-1089?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1070]: https://warthogs.atlassian.net/browse/JUJU-1070?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1099]: https://warthogs.atlassian.net/browse/JUJU-1099?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1109]: https://warthogs.atlassian.net/browse/JUJU-1109?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[JUJU-1091]: https://warthogs.atlassian.net/browse/JUJU-1091?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@barrettj12 barrettj12 mentioned this pull request Jun 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameinel jameinel jameinel left review comments

@juanmanuel-tirado juanmanuel-tirado juanmanuel-tirado approved these changes

Assignees
No one assigned
Labels
2.9 bug The PR addresses a bug
Projects
None yet
Milestone
No milestone
4 participants
@hmlanigan @jameinel @juanmanuel-tirado @jujubot