roll out reporting #1559
roll out reporting #1559
Conversation
Signed-off-by: Mudassar Shafique <15931574+mudash@users.noreply.github.com>
|
Deploy preview for chef-workstation processing. Building with commit b41211e https://app.netlify.com/sites/chef-workstation/deploys/600f10a511ba500007e5234a |
|
In |
| @@ -47,6 +49,21 @@ func main() { | |||
| case "report", "capture": | |||
| cmd = exec.Command(dist.AnalyzeExec, allArgs...) | |||
|
|
|||
| case "push", "push-archive": | |||
| cmd = exec.Command(dist.WorkstationExec, allArgs...) | |||
There was a problem hiding this comment.
Let's update this so that if Rollouts are enabled but not configured correctly in validateRolloutSetup, we fail with the error message before we perform the push. When CHEF_AC_ROLLOUT_ENABLED is set, the operator intent of using Rollouts is clear - so if it's not possible to use them, it should be an error.
There was a problem hiding this comment.
That sounds good. I have addressed it so we will error out and will not do the policy push if the required setup isn't complete.
components/main-chef-wrapper/main.go
Outdated
| runCmd(cmd) | ||
| if validateRolloutSetup() { | ||
| policyGroup := fmt.Sprintf("-g%s", allArgs[1]) | ||
| policyLockFile := fmt.Sprintf("-l%s", allArgs[2]) |
There was a problem hiding this comment.
formatted strings should probably be quoted in output, in case of something like a filename with spaces.
There was a problem hiding this comment.
Used a different approach in the updates now.
Signed-off-by: Mudassar Shafique <15931574+mudash@users.noreply.github.com>
This mechanism was sitting there from original version, I have changed it to route to the default case. |
Signed-off-by: Mudassar Shafique <15931574+mudash@users.noreply.github.com>
…/chef-workstation into mshafique/rollout-reporting
| runCmd(cmd) | ||
| allArgs = []string{"report-new-rollout", "-g", allArgs[1], "-l", allArgs[2], | ||
| "-s", os.Getenv("CHEF_AC_SERVER_URL"), "-u", os.Getenv("CHEF_AC_SERVER_USER")} | ||
| cmd = exec.Command(dist.AutomateCollectExec, allArgs...) |
There was a problem hiding this comment.
For future iterations, I wonder if we can do this more transactionally - first, create the rollout record successfully in automate, then update the results after we execute the command. This way even if the final update fails, we're guaranteed to have some record of the rollout before we proceed to push the policy.
| @@ -0,0 +1,69 @@ | |||
| // | |||
| // Copyright 2019 Chef Software, Inc. | |||
There was a problem hiding this comment.
Copyright © 2021 Progress Software Corporation and/or its subsidiaries or affiliates. All Rights Reserved.
There was a problem hiding this comment.
Updated. Good reminder :)
|
|
||
| func Test_validateRolloutSetup_Invalid(t *testing.T) { | ||
|
|
||
| os.Setenv("CHEF_AC_SERVER_URL", "http://testhost") |
There was a problem hiding this comment.
Ideally we'd test for each var being recognized as required, and not just the one.
There was a problem hiding this comment.
Sure, more is better, updated.
There was a problem hiding this comment.
Thanks @mudash . This looks like a good place from which to build. I've left a couple of minor comments inline.
Signed-off-by: Mudassar Shafique <15931574+mudash@users.noreply.github.com>
Signed-off-by: Mudassar Shafique 15931574+mudash@users.noreply.github.com
Description
This includes set of changes that:
Related Issue
Types of changes
Checklist: