Refactor apiserver ToolsFinder() and ToolsGetter() to take a newEnviron func #11291
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
wallyworld
force-pushed
the
refactor-tools-finder
branch
from
619b85a
to
eb15a75
Compare
babbageclunk
approved these changes
Mar 6, 2020
apiserver/common/tools.go
Outdated
| func NewToolsGetter(f state.EntityFinder, c environs.EnvironConfigGetter, s ToolsStorageGetter, t ToolsURLGetter, getCanRead GetAuthFunc) *ToolsGetter { | ||
| return &ToolsGetter{f, c, s, t, getCanRead} | ||
| func NewToolsGetter(f state.EntityFinder, c environs.EnvironConfigGetter, s ToolsStorageGetter, t ToolsURLGetter, getCanRead GetAuthFunc, newEnviron NewEnvironFunc) *ToolsGetter { | ||
| return &ToolsGetter{newEnviron, f, c, s, t, getCanRead} |
There was a problem hiding this comment.
also added struct attr names
apiserver/common/tools.go
Outdated
| @@ -56,8 +57,8 @@ type ToolsGetter struct { | |||
|
|
|||
| // NewToolsGetter returns a new ToolsGetter. The GetAuthFunc will be | |||
| // used on each invocation of Tools to determine current permissions. | |||
| func NewToolsGetter(f state.EntityFinder, c environs.EnvironConfigGetter, s ToolsStorageGetter, t ToolsURLGetter, getCanRead GetAuthFunc) *ToolsGetter { | |||
| return &ToolsGetter{f, c, s, t, getCanRead} | |||
| func NewToolsGetter(f state.EntityFinder, c environs.EnvironConfigGetter, s ToolsStorageGetter, t ToolsURLGetter, getCanRead GetAuthFunc, newEnviron NewEnvironFunc) *ToolsGetter { | |||
There was a problem hiding this comment.
This takes too many args - maybe just make the fields public and construct a struct? Depends on how important it is that the fields aren't monkeyable-with I guess.
There was a problem hiding this comment.
it does but prefer to keep the struct fields private, and also, this ToolsGetter / ToolsFinder stuff needs more extensive refactoring at some point
| @@ -18,7 +19,7 @@ import ( | |||
| ) | |||
|
|
|||
| type Backend interface { | |||
| state.CloudAccessor | |||
| //state.CloudAccessor | |||
There was a problem hiding this comment.
Remove me? Or reinstate I guess.
state/model.go
Outdated
| @@ -589,6 +589,11 @@ func (m *Model) Cloud() string { | |||
| return m.doc.Cloud | |||
| } | |||
|
|
|||
| // CloudValue returns the cloud to which the model is deployed. | |||
| func (m *Model) CloudValue() (jujucloud.Cloud, error) { | |||
There was a problem hiding this comment.
I realise this is probably a bit more work but it would make more sense to call this Cloud() and change the existing Cloud() method to CloudName(), and similarly for CloudCredential[Value]. I guess Goland can do that pretty easily right?
…udCredential()->CloudCredentialTag(), CloudCredentialValue()->CloudCredntial()
wallyworld
force-pushed
the
refactor-tools-finder
branch
from
17cffa7
to
dc54ec7
Compare
|
|
This was referenced Mar 11, 2020
Merged
jujubot
added a commit
that referenced
this pull request
Mar 13, 2020
#11314 ## Description of change Merge 2.7 bringing in these PRs: #11286 speed up juju status by handling units better #11296 fix LP:1860083 machine availability zone #11291 various refactorings (method renames, tools finder/getter handle k8s broker) #11297 juju show-action-status not listing all #11300 k8s controllers can start lxd containers #11304 protect against nil profile from the charm #11310 speed up processing of machines in juju status #11306 ensure k8s juju upgrades select correct agent version #11313 fix watcher race in ControllerAddressSuite #11302 fix LP:1866658 jujud called in makefile before it is built ## QA steps Run unit tests
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.
Description of change
The apiserver ToolsGetter and ToolsFnder helpers were constructing an IAAS Environ directly. This change refactors them to take a newEnviron func so that the relevant instance for the model can be constructed. The change allows a k8s model to ask for agent binary metadata even though the agent binaries are not used directly.
Part of the change involved fixing the situation where EnvironConfigGetter had both a State and a Model. Only a model is used now (and an interface at that). As more methods are moved/copied to Model from State, we can eventually avoid using State other than to wrap mgo connections.
QA steps
bootstrap an aws controller with --agent-version=2.7.2
deploy postgresql with an ebs volume
(ensures storage registry in state works)
run upgrade-controller --dry-run
(check that 2.7.3 is suggested)
upgrade controller
run upgrade-model --dry-run
(check that 2.7.3 is suggested)
upgrade model