Merge 2.9 20230313 #15279
Merged
Merge 2.9 20230313 #15279
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
… model migration;
juju#15264 This PR fixes two issues failed migration for models with sidecar applications; - https://bugs.launchpad.net/juju/+bug/2009566 - https://bugs.launchpad.net/juju/+bug/2008744 ## 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 ```sh juju deploy prometheus-k8s --trust juju deploy cs:~juju/mariadb-k8s-3 juju migrate k1:t1 k2 Migration started with ID "d2254384-b25c-492b-8e69-798f66e2821c:0" ``` ## Documentation changes No ## Bug reference - https://bugs.launchpad.net/juju/+bug/2009566 - https://bugs.launchpad.net/juju/+bug/2008744 -
juju#15274 Rephrase the error message for less confusion. ## Bug reference https://bugs.launchpad.net/juju/+bug/2009762
* Add removallog for user entries and permit to add previously removed users.
wallyworld
approved these changes
Mar 13, 2023
|
/merge |
ycliuhw
commented
Mar 13, 2023
| // updateExistingUser manipulates the values of an existing user in the db. | ||
| // This is particularly useful when reusing existing users that were previously | ||
| // deleted. | ||
| func (st *State) updateExistingUser(u *User, displayName, password, creator string, secretKey []byte) (*User, error) { |
There was a problem hiding this comment.
@juanmanuel-tirado
The TestAddRemovedUser failed in 3.0:
failed to create user: transaction aborted
There might be some assertion failed in this method.
I don't have enough context for this change.
It'd be great if you could fix it by pushing to this PR directly.
* When recreating the user permissions for a removed user, the transactions are failing. This behaviour does not happen in 2.9. I split the transactions in two different sets.
|
|
|
/build |
|
I'm going to ignore the failing CLA and merge. |
Merged
juanmanuel-tirado
added a commit
that referenced
this pull request
Mar 13, 2023
* Do not increment CharmModifiedVersion for importing a resource during model migration; * Patch statefulset valid fields only; * Fix TestUpgradeStateful; * Rephrase the error message in caas provisioner worker for less confusion; * [JUJU-2928] fully remove users (#15266) * Add removallog for user entries and permit to add previously removed users. * Split transactions when regenerating previously removed user (#26) * When recreating the user permissions for a removed user, the transactions are failing. This behaviour does not happen in 2.9. I split the transactions in two different sets. --------- Co-authored-by: Kelvin <kelvin.liu@canonical.com> Co-authored-by: Juju bot <jujubot@users.noreply.github.com>
Merged
jujubot
added a commit
that referenced
this pull request
Mar 27, 2023
#15346 This merges: - #15268 - #15275 - #15276 - #15269 - #15279 - #15283 - #15305 - #15308 (but undone manually) Conflicts: - api/client/usermanager/client_test.go - apiserver/facades/controller/crossmodelrelations/crossmodelrelations_test.go - apiserver/facades/controller/crossmodelrelations/mock_test.go - apiserver/package_test.go - container/lxd/initialisation_linux.go - scripts/win-installer/setup.iss - snap/snapcraft.yaml - version/version.go Resolved trivially
hpidcock
reviewed
Mar 29, 2023
state/user.go
Outdated
| @@ -186,7 +191,6 @@ func (st *State) updateExistingUser(u *User, displayName, password, creator stri | |||
| Assert: txn.DocExists, | |||
| Update: updateUser, | |||
| }} | |||
| updateUserOps = append(updateUserOps, removeControllerOps...) | |||
There was a problem hiding this comment.
These really need to done in the same transaction. The problem here is that there is no get to check the the controller even has this user before adding the ops from removeControllerUserOps.
1 task
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.
Merge 2.9
Conflicts were for deleted code or code that needed to be removed.