fix: wire up EnvironConfigGetter to use model config service #18028
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
hmlanigan
force-pushed
the
model-config-service-into-ecg
branch
6 times, most recently
from
979d7cc
to
22aa7e0
Compare
|
/build |
hmlanigan
force-pushed
the
model-config-service-into-ecg
branch
2 times, most recently
from
68fc108
to
4a067fe
Compare
hmlanigan
force-pushed
the
model-config-service-into-ecg
branch
6 times, most recently
from
60c5f8c
to
f856fc1
Compare
hmlanigan
force-pushed
the
model-config-service-into-ecg
branch
from
f856fc1
to
fa89117
Compare
This allows for facade tests which double write data to share a model UUID between state and the service factory setup. Needed to start with getting ModelConfig, if the requested model state does not exist in a service factory, the environ config getters will fail during test.
This is a stop gap on the way to reducing dependence on Environs and replacing with appropriate calls into the different domain services where possible. That work was started in PR 18013. GetNewPolicyFunc needs a model config service getter. Callers do not have a model at the setup time of GetNewPolicyFunc. Use a model config service getter instead. Special case the model config service getter with the environStatePolicy structure to limit use and not require everywhere, using a model config service is preferable. A short term solution one the way to removing the Policy's all together. Delete stateenvirons NewInstancePrechecker and associated code. The functionality has been replace and no longer required here. Remove configvalidator_test.go. These are model config tests which are failing in state after changes to the EnvironConfigGetter. We're near the end of the work to move model config from state to a domain. Thus they can be removed now rather than fixing.
hmlanigan
force-pushed
the
model-config-service-into-ecg
branch
from
fa89117
to
0ab72ba
Compare
| @@ -368,18 +360,14 @@ func (s *bootstrapSuite) TestInitializeStateFailsSecondTime(c *gc.C) { | |||
| AdminUser: adminUser, | |||
| StateInitializationParams: args, | |||
| MongoDialOpts: mongotest.DialOpts(), | |||
| StateNewPolicy: state.NewPolicyFunc(nil), | |||
There was a problem hiding this comment.
Surprised this can go as well.
Comment on lines
-397
to
-404
| StateNewPolicy: stateenvirons.GetNewPolicyFunc( | ||
| cloudGetter{cloud: &args.ControllerCloud}, | ||
| credentialGetter{cred: args.ControllerCloudCredential}, | ||
| // We don't need the storage service at bootstrap. | ||
| func(modelUUID model.UUID, registry storage.ProviderRegistry) state.StoragePoolGetter { | ||
| return noopStoragePoolGetter{} | ||
| }, | ||
| ), |
There was a problem hiding this comment.
I'm trying to work out if we're at liberty to do this. The policy func is also required for config and constraints validation.
There was a problem hiding this comment.
I removed it due to a comment in agent/agentbootstrap/bootstrap.go:
// Deprecated: use InstancePrechecker
StateNewPolicy state.NewPolicyFunc
InstancePrecheckerGetter func(*state.State) (environs.InstancePrechecker, error)
9fe3ea8
to
86b486a
Compare
In the process of removing model config from state, there is functionality which needs to be skipped for now until it can be re-implemented in a domain. In this case skip validation model constraints for now. The model domain will be completed after model config. Skip 2 tests which fail due to above change. Remove once they have been handled in the model epic.
Over zealous with instance prechecker removal from stateenviron. These tests will work and the instance prechecker will be reimplmented in the machine domain.
hmlanigan
force-pushed
the
model-config-service-into-ecg
branch
from
86b486a
to
6877789
Compare
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.
For Environs, ensure that Config is provided by the ModelConfigService rather than state. This is a stop gap on the way to reducing dependence on Environs and replacing with appropriate calls into the different domain services where possible.
The above changed required that NewCAASBrokerFunc and NewEnvironFunc were also updated to allow in a ModelConfigService.
In the process of this work, it became clear it should be done in conjunction with removing the NewInstancePrechecker to avoid unnecessary work when it was removed next. The InstancePrechecker will be replaced in the machine service.
Checklist
[ ] 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 packagesQA steps
This change touches many pieces of juju around deploy and providers. Test by using bootstrap, deploy, model config, refresh, status and destroy in different ways. Unfortunately the bash tests do not run easily for unrelated reasons.
Model migration is tested via the GitHub Actions.
Test updating model config
Links
Jira card: JUJU-6674
Jira card: JUJU-6687