Allow user to add manual approval for deploy jobs.
liweiyi88 opened this issue · 9 comments
It would be very useful to add an environment attribute in the workflow yml file for deploy jobs. Then it will enable user to config manual approval if needed.
How it works.
We add an environment section in the workflow file like below
jobs:
cdk_diff:
runs-on: ubuntu-latest
steps:
...
deploy_to_dev:
needs: cdk_diff
environment: <--- environment for deploying to development job
name: development
runs-on: ubuntu-latest
steps:
...
deploy_to_staging:
needs: deploy_to_dev
environment: <--- environment for deploying to staging job
name: staging
runs-on: ubuntu-latest
steps:
...
At this stage, those extra environment attributes won't introduce any side effects or breaking changes.
Secondly, in github -> settings -> environment page, we create two github.com environments with the same names (development and staging) like below
After this step, nothing won't happen until we configure the environment like the below screenshot.
Here, in the staging environment, I ticked the required reviewers and assign my username to it. after saving the change, the workflow will block in the deploy to staging step and ask me to approve it.
Here is the working repository
So in #160, I added the environment attribute based on the stage name for the deploy jobs. Then it will give the user the ability to add manual approval if needed.
Please let me know if you have any further questions.
Hi @liweiyi88! I'm not actually worried about side effects or breaking changes -- but I want to focus on the user experience and unlock as many use cases as possible from this feature.
The detail you've provided is invaluable to me. Thank you for that. This is absolutely something this module should support, the question is what is the best way to do that. Especially because manual approval is something that exists in the AWS CodePipeline counterpart in @aws-cdk/pipelines.
My concern with your solution is that you have to know what you're doing in order to achieve this functionality. The discoverability of this feature will suffer, and less people will stumble across it and utilize it as a result.
My second concern is, what if a user does not want their environment to be the stack name? Again, I'm not overly familiar with the use case because I've never used environments before, but I feel like this could be the case. Also, I don't think it's feasible to simply hard code the environment for every deploy and expect that users are okay with the value being ignored if they are not using it -- in my mind, everything in deploy.yml should be asked for.
To assuage these concerns, I suggest:
- perhaps we should expose a variable like
deployInGitHubEnvironment: booleanthat toggles this feature on and off. In the docstring, we can also describe what this entails and why its useful. We can add it to the readme as well. - i understand that we are kind of hamstrung by the fact that some of the pipeline logic lives in
@aws-cdk/pipelines. However, we should think about what people might want to name their environments. Perhaps it makes more sense to utilize the tags feature on a stack. We might be able to get away with saying that we search for a tag keyedgithub-envand name the environment that if we can find it.
// user might write this
const fnStack = new Stack(this, 'FunctionStack');
fnStack.tags.setTag('github-env', 'production');// in pipeline.ts
environment: {
name: stack.tags['github-env'] ? stack.tags['github-env'] : stack.stackName,
},Lastly, in an ideal world we could some how incorporate the ManualApprovalStep that is available in @aws-cdk/pipelines. That would be the ultimate user experience. I can't see how we can in this case, but if you can imagine a world where it's possible, please let me know!
@liweiyi88, I think this is a great idea but the execution is a one-way door (even though this module is experimental :)). We should be very deliberate in determining the best path forward here. Let me know what you think!
@kaizen3031593 Thanks for the feedback. I would like to add some comments in regard to the points.
- Yes, I agree it would be better to have a feature toggle. What about using env var instead of a normal variable to avoid passing the variable around from parent to nested function?
- The proposed solution was to use the stage name defined by the user and pass it to the environment section.
const myStage = new MyStage(this, 'StageA', { env: EnvironmentUtils.parse(props.envA) });
pipeline.addStage(myStage, {
...
});
So users will have their own control over the name passed to the environment section. However, I couldn't find a better way to get the stage name from the stackDeployment but to get that by extracting the stage name from the stack name.
environment: {
name: stack.stackName.split('-', 1).join(),
},
...
It would be better if you could shed some light on a better way to get the stage name?
3. So point 2 will introduce another design question: Shall we gate the stage or stack? If we gate the stack I would prefer your suggestion of using stack tags to overwrite the name. But IMO, gating the stage feels more natural and consistent from a users' perspective and expectation. When we talk about the deploy stages we usually refer to deploying stacks to different environments, this is in most cases an atom action (we deploy the whole stack to the staging environment if failed we rollback all of them). I do not have a very strong opinion on this as I do also feel providing fine-grained control on gating on stack level gives users the max flexibility. Please let me know your thoughts?
4. I wasn't aware that ManualApprovalStep is a thing in @aws-cdk/pipelines, I will have a look.
perhaps we should expose a variable like deployInGitHubEnvironment: boolean that toggles this feature on and off. In the docstring, we can also describe what this entails and why its useful. We can add it to the readme as well.
i understand that we are kind of hamstrung by the fact that some of the pipeline logic lives in @aws-cdk/pipelines. However, we should think about what people might want to name their environments. Perhaps it makes more sense to utilize the tags feature on a stack. We might be able to get away with saying that we search for a tag keyed github-env and name the environment that if we can find it.
I think we could combine the 2-steps into one by only considering if the stack has a tag github-env without introducing a separate variable for feature toggle.
In this way, we add the github environment section to the yml file only when we find the tag and let the end-users to control the environment name.
@liweiyi88, thanks for being proactive on this! Give me a couple days to think about this and talk to some people. I can't think of a better idea (yet) but I'm not fully certain that this is a reasonable use case for tags.
@kaizen3031593 , having an extra field in StackDeployment or getting the environment name from context may be the other 2 options but I am not sure these 2 are better than tagging (IMO, tagging is fine and AWS uses tagging to enable features for EKS related components as well).
Anyway, thanks for your feedback and let me know once you have a better idea.
I agree :). But I want to touch base with @rix0rrr just to be sure. Expect an update early next week with the go-ahead or with some new ideas! Appreciate the patience.
Hi @liweiyi88. Had a chat this morning about this and wanted to update you on the direction we're looking to take.
First, we're going to move away from the tagging solution. It's just not that elegant and we don't want to rely on tagging for every github-specific property we want to add. second, if we think about the environment variable, it's something that should exist at the Stage level, not the Stack level. A Stage is a collection of stacks to be deployed in the same way, and that's the correct grouping for adding an environment variable (basically, I don't see a reason why someone would want stacks in the same stage and not the same github environment). Even though we see things at a Stack granularity in workflow.yml, adding stack-level github environments would leak the abstraction to a different layer. We want to keep the abstraction tight and expose this on the Stage level.
What does that mean? I've created a new api for a addStageWithGitHubOpts() that allows you to specify stage-level options pertaining to github. Right now, it's only the environment, but we can add more there in the future (where it would be tough to do so with tags). Originally, I wanted to just override addStage, but there were some JSII compilation errors that prevented that from happening.
Since it's very different than what we were talking about and I had to scope it out, I ended up doing it myself. I hope you understand! I added a link to this discussion in the README as well as one of your screenshots (hopefully reviewers will let that one fly, but I'll try)