-
Notifications
You must be signed in to change notification settings - Fork 510
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
refactor: call ModelConfigAPI instead of ControllerAPI.ModelConfig #17732
Conversation
/build |
47bab0d
to
8b11d80
Compare
/build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I have run all the tests I can think of and this looks good.
Next step will be to cut a new facade version in 3.6 that removes this method and then we can nurse that all the way up into 4.0.
I have checked the Terraform provider wasn't using this. Python may use this so I think cut a new version in 3.6 removing the command and that covers our bases. We can then just support the new version only in 4.0.
Affected commands are upgrade-model, destroy-controller, kill-controller, show-controller.
8b11d80
to
52dc8d7
Compare
/merge |
v12 has removed the ModelConfig method. This is in preparation for 4.0 where we will change this facade to a multi-model facade. Calls to this facade method were already removed in juju#17732.
v12 has removed the ModelConfig method. This is in preparation for 4.0 where we will change this facade to a multi-model facade. Calls to this facade method were already removed in juju#17732.
v12 has removed the ModelConfig method. This is in preparation for 4.0 where we will change this facade to a multi-model facade. Calls to this facade method were already removed in juju#17732.
#17761 v12 has removed the ModelConfig method. This is in preparation for 4.0 where we will change this facade to a multi-model facade. Calls to this facade method were already removed in #17732. ## Checklist <!-- If an item is not applicable, use `~strikethrough~`. --> - [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/main/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 Bootstrap Juju, run `show-controller`, and check that the controller model version displays correctly: ``` $ juju show-controller | grep controller-model-version controller-model-version: 3.6-beta2.1 ``` Run `upgrade-model` on the controller model, and check that it raises an error: ``` $ juju switch controller $ juju upgrade-model ERROR use upgrade-controller to upgrade the controller model ``` Run `destroy-controller` and check that the controller is successfully destroyed. Bootstrap Juju again, and add the 3.6 controller to a 3.5 client: ``` juju3.6:~$ juju add-user juju35 User "juju35" added Please send this command to juju35: juju register ... juju3.6:~$ juju grant juju35 superuser juju3.6:~$ juju grant juju35 admin controller ``` ``` juju3.5:~$ juju register ... ``` Now do all the above steps again with the 3.5 client, to ensure compatibility between 3.5 and 3.6. ## Links **Jira card:** JUJU-6351
- The `Controller` facade has become a multi-model facade. - It now uses the following services: ModelService, ModelConfigService (for each model), AgentService - Controller facade v11 has been removed, only v12 is supported in 4.0. The ModelConfig method no longer exists. All client methods are now using the ModelConfig facade instead (see juju#17732). - Controller facade tests are slowly being ported over to using mocked services. - The service factory now has methods `ModelAgent` which returns a model-scoped agent service, and `Agent` which returns a general agent service to access the agent version for any model. A `ControllerAgentVersion` method has been added for convenience. - The ModelStatus api client decodes its errors using `params.TranslateWellKnownError`. - Added a `CreateTestModelWithConfig` method to control parameters for created test models.
- The `Controller` facade has become a multi-model facade. - It now uses the following services: ModelService, ModelConfigService (for each model), AgentService - Controller facade v11 has been removed, only v12 is supported in 4.0. The ModelConfig method no longer exists. All client methods are now using the ModelConfig facade instead (see juju#17732). - Controller facade tests are slowly being ported over to using mocked services. - The service factory now has methods `ModelAgent` which returns a model-scoped agent service, and `Agent` which returns a general agent service to access the agent version for any model. A `ControllerAgentVersion` method has been added for convenience. - The ModelStatus api client decodes its errors using `params.TranslateWellKnownError`. - Added a `CreateTestModelWithConfig` method to control parameters for created test models.
This PR is prerequisite work to a change which will come in 4.0. While attempting to move the
Controller
facade over to model services, we realised that the controller facade should be a multi-model facade. Currently it is a model-scoped facade. However, we can't change it over right now due to theModelConfig
facade method, which requires the facade to be scoped to a single model.So in this PR, we replace all usages of
Controller.ModelConfig
with theModelConfig
facade. This ensures that 3.6 clients will continue to be compatible with 4.0 controllers when we change theController
facade in 4.0.This PR does not include any facade changes, but we have removed the
ModelConfig
method from the controller API client to discourage people from using that facade method. Furthermore, we have added a deprecation warning to the facade method. In 4.0, we will remove this facade method and bump the facade version.Checklist
[ ] Comments saying why design decisions were made[ ] Integration tests, with comments saying what you're testing[ ] doc.go added or updated in changed packagesQA steps
Affected commands are
upgrade-model
,destroy-controller
,kill-controller
,show-controller
. The following should test all of the code paths that have been modified.Bootstrap Juju, run
show-controller
, and check that the controller model version displays correctly:Run
upgrade-model
on the controller model, and check that it raises an error:Run
destroy-controller
and check that the controller is successfully destroyed.Bootstrap Juju again, run
kill-controller
and check that the controller is successfully destroyed.Links
Jira card: JUJU-6350