Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Added JUJU_SLA environment variable to the hook context. #7109
Conversation
|
!!test!! |
jameinel
requested changes
Mar 20, 2017
At the APIServer level we should be exposing just the function like "SLALevel()" on the Uniter Facade, and it can do whatever is necessary to get the right value to return.
| @@ -31,7 +31,7 @@ type ModelConfigAPI struct { | ||
| // NewModelConfigAPI creates a new instance of the ModelConfig Facade. | ||
| func NewModelConfigAPI(backend Backend, authorizer facade.Authorizer) (*ModelConfigAPI, error) { | ||
| - if !authorizer.AuthClient() { | ||
| + if !authorizer.AuthClient() && !authorizer.AuthUnitAgent() { |
jameinel
Mar 20, 2017
Owner
This doesn't look correct. The UnitAgent already has its own API Facade to use. Instead of trying to use a different Facade, you should be adding the function that you need on its Facade. (Uniter).
ModelConfig is the Client (aka User) API, it should not be used by Agents.
Even further, you should not be asking for all of ModelConfig, you should be calling a function that only returns the details you actually need.
It might be backed by ModelConfig (that is where it gets the data), but it should not be asking for everything-that-could-be-configured-ever. Otherwise when we refactor where data is stored, it requires auditing for "you're asking for a giant map, what bits do you actually depend on".
| @@ -89,6 +90,8 @@ func Manifold(config ManifoldConfig) dependency.Manifold { | ||
| return nil, errors.Errorf("expected a unit tag, got %v", tag) | ||
| } | ||
| uniterFacade := uniter.NewState(apiConn, unitTag) | ||
| + modelConfigFacade := modelconfig.NewClient(apiConn) |
jameinel
Mar 20, 2017
Owner
again, it should be a new API on the 'uniterFacade', not accessing the client facade.
| + | ||
| +// SLALevelGetter defines an interface the context factory uses to retrieve | ||
| +// the current SLA level. | ||
| +type SLALevelGetter interface { |
jameinel
Mar 20, 2017
Owner
This abstraction is fine if it is helpful, but if it is on uniterFacade, you probably don't need it.
|
@jameinel PTAL |
| @@ -48,6 +49,22 @@ func (s *modelconfigSuite) SetUpTest(c *gc.C) { | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| } | ||
| +func (s *modelconfigSuite) TestAccessDenied(c *gc.C) { |
wallyworld
Mar 21, 2017
Owner
This test isn't really part of this PR anymore now that the methods have been correctly added to the uniter facade.
Just delete the test IMO
| @@ -1685,3 +1685,13 @@ func (u *UniterAPIV3) getOneNetworkConfig(canAccess common.AuthFunc, unitTagArg, | ||
| return results, nil | ||
| } | ||
| + | ||
| +// SLALevel returns the model's SLA level. | ||
| +func (u *UniterAPIV3) SLALevel() (params.StringResult, error) { |
wallyworld
Mar 21, 2017
Owner
So, we are adding a new API to the uniter facade, but to do this right, the version needs to be bumped. And the api client needs to check what backend version it is talking to and fail gracefully if the agent has not yet been upgraded to the new v4 facade. There's a couple of examples of how to do this floating around in the codebase.
| @@ -89,6 +89,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold { | ||
| return nil, errors.Errorf("expected a unit tag, got %v", tag) | ||
| } | ||
| uniterFacade := uniter.NewState(apiConn, unitTag) | ||
| + |
| @@ -579,6 +582,7 @@ func (context *HookContext) HookVars(paths Paths) ([]string, error) { | ||
| "JUJU_API_ADDRESSES="+strings.Join(context.apiAddrs, " "), | ||
| "JUJU_METER_STATUS="+context.meterStatus.code, | ||
| "JUJU_METER_INFO="+context.meterStatus.info, | ||
| + "JUJU_SLA="+context.slaLevel, |
wallyworld
approved these changes
Mar 21, 2017
Looks good.
One suggestion - rename NewUniterAPIV4 to simply NewUniterAPI as there are various typos in the comments around the place referring to the wrong version number. 3 vs 4 vs 5 etc
| @@ -29,6 +29,9 @@ var logger = loggo.GetLogger("juju.apiserver.uniter") | ||
| func init() { | ||
| common.RegisterStandardFacade("Uniter", 4, NewUniterAPIV4) | ||
| + | ||
| + // Version 5 introduces the SLA levels. | ||
| + common.RegisterStandardFacade("Uniter", 5, NewUniterAPIV4) |
|
!!test!! |
|
!!test!! |
| @@ -53,8 +56,8 @@ type UniterAPIV3 struct { | ||
| StorageAPI | ||
| } | ||
| -// NewUniterAPIV4 creates a new instance of the Uniter API, version 3. | ||
| -func NewUniterAPIV4(st *state.State, resources facade.Resources, authorizer facade.Authorizer) (*UniterAPIV3, error) { | ||
| +// NewUniterAPI creates a new instance of the Uniter API, version 3. |
|
!!test!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
alesstimec commentedMar 16, 2017
•
Edited 1 time
-
alesstimec
Mar 16, 2017
Description of change
This adds the JUJU_SLA environment variable to the hook context so that charm authors can see the current SLA level for the model.
QA steps
Documentation changes
The new JUJU_SLA environment variable needs to be documented. Speak to @cmars for details
Bug reference
n/a