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

refactor: call ModelConfigAPI instead of ControllerAPI.ModelConfig #17732

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

barrettj12
Copy link
Contributor

@barrettj12 barrettj12 commented Jul 15, 2024

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 the ModelConfig 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 the ModelConfig facade. This ensures that 3.6 clients will continue to be compatible with 4.0 controllers when we change the Controller 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

  • Code style: imports ordered, good names, simple structure, etc
  • [ ] Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • [ ] Integration tests, with comments saying what you're testing
  • [ ] doc.go added or updated in changed packages

QA 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:

$ 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, run kill-controller and check that the controller is successfully destroyed.

Links

Jira card: JUJU-6350

@barrettj12 barrettj12 marked this pull request as ready for review July 15, 2024 00:54
@hpidcock hpidcock added the 3.6 label Jul 15, 2024
@barrettj12
Copy link
Contributor Author

/build

@barrettj12 barrettj12 requested a review from tlm July 16, 2024 02:30
@barrettj12 barrettj12 force-pushed the controller-modelcfg branch from 47bab0d to 8b11d80 Compare July 16, 2024 23:10
@barrettj12
Copy link
Contributor Author

/build

Copy link
Member

@tlm tlm left a 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.

cmd/juju/commands/upgradecontroller.go Show resolved Hide resolved
cmd/juju/controller/kill.go Show resolved Hide resolved
cmd/juju/controller/showcontroller.go Outdated Show resolved Hide resolved
Affected commands are upgrade-model, destroy-controller,
kill-controller, show-controller.
@barrettj12 barrettj12 force-pushed the controller-modelcfg branch from 8b11d80 to 52dc8d7 Compare July 17, 2024 01:49
@barrettj12
Copy link
Contributor Author

/merge

@jujubot jujubot merged commit 28e4763 into juju:3.6 Jul 17, 2024
21 of 23 checks passed
@barrettj12 barrettj12 deleted the controller-modelcfg branch July 17, 2024 02:49
barrettj12 added a commit to barrettj12/juju that referenced this pull request Jul 18, 2024
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.
barrettj12 added a commit to barrettj12/juju that referenced this pull request Jul 18, 2024
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.
barrettj12 added a commit to barrettj12/juju that referenced this pull request Jul 18, 2024
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.
jujubot added a commit that referenced this pull request Jul 21, 2024
#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
barrettj12 added a commit to barrettj12/juju that referenced this pull request Jul 25, 2024
- 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.
barrettj12 added a commit to barrettj12/juju that referenced this pull request Jul 29, 2024
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants