Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add draft CI workflow #1

Merged
merged 16 commits into from
Apr 15, 2021
Merged

Add draft CI workflow #1

merged 16 commits into from
Apr 15, 2021

Conversation

BrunoGrandePhD
Copy link
Contributor

@BrunoGrandePhD BrunoGrandePhD commented Apr 10, 2021 •
edited
Loading

I'm not sure what's the best way of testing these CI workflows other than reviewing them and making sure they behave as expected once merged.

I've opted for a setup where we have two stack groups: dev and prod. The dev stacks will be deployed to the workflows-nextflow-dev account whereas the prod stacks will be deployed to the workflows-nextflow-dev and workflows-nextflow-prod accounts. Another pattern I noticed here is to have dev, prod, and common.

You can ignore the change affecting the CODEOWNERS file. I just removed the extra line at the end of the file to satisfy the pre-commit test.

@BrunoGrandePhD BrunoGrandePhD changed the base branch from main to develop April 10, 2021 05:00
@BrunoGrandePhD BrunoGrandePhD requested a review from tthyer April 12, 2021 23:52
@BrunoGrandePhD
Copy link
Contributor Author

I'll request a review from @zaro0508 once he accepts the invitation to join this GitHub organization, which I just issued.

@BrunoGrandePhD BrunoGrandePhD changed the base branch from develop to main April 12, 2021 23:57
@BrunoGrandePhD BrunoGrandePhD changed the base branch from main to develop April 12, 2021 23:57
@BrunoGrandePhD
Copy link
Contributor Author

I played with the base branch to force-update the diffs, which were stuck on comparing with the old develop branch (before I made it up-to-date with main).

tthyer
tthyer suggested changes Apr 13, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
zaro0508
zaro0508 requested changes Apr 13, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
@BrunoGrandePhD BrunoGrandePhD requested a review from tthyer April 13, 2021 16:52
@BrunoGrandePhD
Copy link
Contributor Author

@zaro0508: Ready for a second round of review if you want.

zaro0508
zaro0508 approved these changes Apr 13, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
@BrunoGrandePhD BrunoGrandePhD changed the base branch from develop to dev April 13, 2021 18:00
@BrunoGrandePhD BrunoGrandePhD changed the base branch from dev to main April 13, 2021 21:07
@BrunoGrandePhD
Copy link
Contributor Author

For the time being, I propose that we set up this repository to only deploy to the production AWS account. We can reserve the development account for testing templates using manual sceptre deployment.

I've simplified the CI workflow in this PR accordingly. I'm open to suggestions on how to further improve this workflow.

/cc @zaro0508 @tthyer

@BrunoGrandePhD BrunoGrandePhD changed the base branch from main to prod April 13, 2021 22:47
zaro0508
zaro0508 requested changes Apr 14, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
tthyer
tthyer approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@tthyer tthyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than perhaps also using the newly discovered sceptre action, this LGTM

zaro0508
zaro0508 requested changes Apr 14, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
tthyer
tthyer suggested changes Apr 14, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
zaro0508
zaro0508 requested changes Apr 14, 2021
View reviewed changes
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
@BrunoGrandePhD
Copy link
Contributor Author

Now that Sage-Bionetworks-IT/organizations-infra#141 has been merged, I've updated the service user credentials in the organization secrets accordingly. I'm also using the aws-actions/configure-aws-credentials@v1 action here to assume the service role that was created. From what I can tell, this action should be compatible with the sceptre action. 🤞

@zaro0508 @tthyer: Feel free to take one last review.

tthyer
tthyer approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@tthyer tthyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

zaro0508
zaro0508 approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@zaro0508 zaro0508 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks like the best option yet. very nice and neat workflow.

@BrunoGrandePhD BrunoGrandePhD merged commit b5b8411 into prod Apr 15, 2021
@BrunoGrandePhD BrunoGrandePhD deleted the bgrande/enable-ci branch April 15, 2021 00:03
@zaro0508
Copy link
Contributor

@thomasyu888 should really take a look at this PR

@tthyer
Copy link
Contributor

tthyer commented Apr 15, 2021

+1 there are several new actions that will be good to apply in our templates and ci examples, too

BrunoGrandePhD pushed a commit that referenced this pull request Sep 10, 2022
@adamjtaylor
Merge pull request #1 from Sage-Bionetworks-Workflows/main
d5d44a5 
Update to latest
BWMac pushed a commit that referenced this pull request Jan 3, 2025
@beatrizsaldana
Merge pull request #1 from beatrizsaldana/beatrizsaldana-create-tower…
bf104f3 
…-project

Create diverse-cohorts-project-beatriz.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tthyer tthyer tthyer approved these changes

@zaro0508 zaro0508 zaro0508 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BrunoGrandePhD @zaro0508 @tthyer