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 a pre/post-deploy steps to budle deploy for metric registration. #8175
Conversation
|
I think you want
!!build!!
John
=:->
…
|
|
!!build!! |
|
!!build!! |
| + // `"hello registration"\n` (quotes and newline from json | ||
| + // encoding) is returned by the fake http server. This is binary64 | ||
| + // encoded before the call into SetMetricCredentials. | ||
| + //creds := append([]byte(`"aGVsbG8gcmVnaXN0cmF0aW9u"`), 0xA) |
| + _, err := cmdtesting.RunCommand(c, deploy, "bundle/wordpress-with-plans") | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + //c.Check(setMetricCredentialsCall(), gc.Equals, 1) |
| @@ -496,6 +496,9 @@ type DeployStep interface { | ||
| // RunPre runs before the call is made to add the charm to the environment. | ||
| RunPre(MeteredDeployAPI, *httpbakery.Client, *cmd.Context, DeploymentInfo) error | ||
| + // RunPreBundle runs before the call is made to add a bundle charm to the environment. | ||
| + RunPreBundle(MeteredDeployAPI, *httpbakery.Client, *cmd.Context, DeploymentInfo) error |
tasdomas
Dec 7, 2017
Member
It looks like the purpose of RunPreBundle is to perform charm authorization without checking if the charm is metered. Is that right?
What is the meaning of obtaining an auth for a non-metered charm, when it's deployed in a bundle?
If we were to leave the check for whether the charm is metered or not, would we be able to get rid of RunPreBundle?
It's just that
- the fact that there's no RunPostBundle and
- that RunPre calls RunPreBundle
make the DeployStep interface difficult to understand anddeploy.gois not the lightest read as far as code goes already.
cmars
Dec 7, 2017
Owner
I'm ok with attempting to obtain the plan specified in the bundle unconditionally; if we get one from the API, it's reasonable to set it IMO. I agree with @tasdomas though that the method name RunPreBundle is confusing.. what does RunPre have to do with bundles? And the logic is getting complicated in there.
I think the general approach in deployBundle is fine, but let's please break up the logic and operations in such a way that they can be reused by both the "deploy charm" and "deploy bundle" flow in a clear and concise way.
| @@ -496,6 +496,9 @@ type DeployStep interface { | ||
| // RunPre runs before the call is made to add the charm to the environment. | ||
| RunPre(MeteredDeployAPI, *httpbakery.Client, *cmd.Context, DeploymentInfo) error | ||
| + // RunPreBundle runs before the call is made to add a bundle charm to the environment. | ||
| + RunPreBundle(MeteredDeployAPI, *httpbakery.Client, *cmd.Context, DeploymentInfo) error |
tasdomas
Dec 7, 2017
Member
It looks like the purpose of RunPreBundle is to perform charm authorization without checking if the charm is metered. Is that right?
What is the meaning of obtaining an auth for a non-metered charm, when it's deployed in a bundle?
If we were to leave the check for whether the charm is metered or not, would we be able to get rid of RunPreBundle?
It's just that
- the fact that there's no RunPostBundle and
- that RunPre calls RunPreBundle
make the DeployStep interface difficult to understand anddeploy.gois not the lightest read as far as code goes already.
cmars
Dec 7, 2017
Owner
I'm ok with attempting to obtain the plan specified in the bundle unconditionally; if we get one from the API, it's reasonable to set it IMO. I agree with @tasdomas though that the method name RunPreBundle is confusing.. what does RunPre have to do with bundles? And the logic is getting complicated in there.
I think the general approach in deployBundle is fine, but let's please break up the logic and operations in such a way that they can be reused by both the "deploy charm" and "deploy bundle" flow in a clear and concise way.
| + ModelUUID: modelUUID, | ||
| + } | ||
| + | ||
| + err = s.RunPreBundle(apiRoot, bakeryClient, ctx, deployInfo) |
cmars
Dec 7, 2017
Owner
Here I'd just look up the plan if "default" with getDefaultPlan, then call registerMetrics, failing immediately on any error. If the bundle author specified a plan, the intent is to deploy as specified, no magic.
| info := deployInfo.CharmInfo | ||
| - if r.Plan == "" && info.Metrics != nil && !info.Metrics.PlanRequired() { | ||
| + if r.Plan == "" && info != nil && info.Metrics != nil && !info.Metrics.PlanRequired() { |
cmars
Dec 7, 2017
Owner
Hmm.. overloading logic here, deployInfo.CharmInfo == nil to mean "ignore the plan required checking logic we'd use otherwise" isn't very clear.
cmars
Dec 7, 2017
Owner
I'd also argue that only RunPre cares about ignoring registerMetrics errors for non-charmstore deployments.
| + return errors.Trace(err) | ||
| + } | ||
| + | ||
| + defer func() { |
mattyw
Dec 19, 2017
Member
I don't really like the pattern of having a defer that closes over an error like this
|
$$merge$$ |
|
$$merge$$ |
|
|
|
$$merge$$ |
|
$$merge$$ |
|
Build failed: Tests failed |
|
$$merge$$ |
jameinel
deleted a comment from
jujubot
Dec 20, 2017
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
alesstimec commentedDec 5, 2017
•
Edited 1 time
-
alesstimec
Dec 5, 2017
Description of change
With the latest changes to the bundle file format, authors are able to specify the plans under which individual applications are to be deployed.
Why is this change needed?
To improve the UX of bundle deployment for bundles that
contain charms that must be deployed under a specific plan.
QA steps
Documentation changes
Need to add docs to describe changes in the bundle format.
Bug reference
N/A