-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add new api to unlock github-specific stage-level properties #178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comments. Feel free to merge afterwards.
src/pipeline.ts
Outdated
// keep track of GitHub specific options | ||
if (options?.gitHubEnvName) { | ||
for (const stack of stageDeployment.stacks) { | ||
this.stackEnvs[stack.stackName] = options.gitHubEnvName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels potentially dangerous--multiple stacks can have the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the key to stack.artifactID
which should be unique within the cloud assembly.
Signed-off-by: github-actions <github-actions@github.com>
@@ -421,6 +461,7 @@ export class GitHubWorkflow extends PipelineBase { | |||
contents: github.JobPermission.READ, | |||
idToken: this.useGitHubActionRole ? github.JobPermission.WRITE : github.JobPermission.NONE, | |||
}, | |||
environment: this.stackEnvs[stack.stackArtifactId], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaizencc, I just tried the new feature and found one thing.
If ppl use the normal AddStage method, the output deploy.yml will have environment: null
. Wonder if we shall skip adding the environment key if this.stackEnvs[stack.stackArtifactId] is null.
Possibliy:
...(this.stackEnvs[stack.stackArtifactId] && {
environment: this.stackEnvs[stack.stackArtifactId],
}),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @julianiag, caught me at a good time lol. Does that create an environment for you when you deploy? Or is it simply ignored? I agree it's not great. Maybe I can change it up tomorrow :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, if you add that in as a PR I will approve it, but you'll have to run yarn build
to update the snapshots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…#186) Based on discussion #178 (comment) Removes `environment: null` from workflows that did not specify the environment.
Fixes #162.
Introduces a new API as follows:
Unlocks GitHub Environments at a stage-level, as well as a blueprint for any future stage-level github-specific properties we want to add.