-
Notifications
You must be signed in to change notification settings - Fork 0
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
Deployment of registry-api and nucleus completed #56
Conversation
udpate pre-commit update pre-commit unmask region oidc error fix retry on failure udpate region again change aws library version change aws library version back aws sdk v3 aws sdk v2.2.0 aws sdk v1.7.0 revert to aws sdk v2 pass region as env var add session name add session name hardcode region update region once again add quotes around env vars udpate workflow and vars update workflow fix error comment out default vars test workflow add dummy values for vars add dummy values for vars add dummy values for vars aws_s3_bucket_object deprecated update pre-commit update vpc id as var revert vpc id back as a secret versioning and encryption for s3 update terraform apply remove vpc value remove vpc cidr value remove ingress cidr value remove dummy values remove dummy variable values fix module variables versioning and encryption for s3 update and test nucleus workflow udpate terraform plan rebasing continued rebasing continued rebasing continued rebasing continued full rebase
branches: | ||
- '**' | ||
pull_request: | ||
types: [opened, edited, reopened, ready_for_review, review_requested] |
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.
@sjoshi-jpl similar question to other TF PRs, do we really want this on branches/PRs or do we just want this on merges to main?
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.
@jordanpadams yes, its generally a good practice to run at least terraform plan
before merging to main because if there are any issues within the terraform code itself, it won't be caught until the code is merged to main.
Additionally, when a developer is testing code changes (for their specific branch), its helpful to be able to view the output of the run within GitHub in order to troubleshoot any issues. So they don't create a PR before those errors are resolved.
If you'd still like to limit the number of times tf plan runs, I can change it to only execute during a push to main and when a PR for main is created.
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.
@sjoshi-jpl bah! didn't read closely enough below where you only run the apply step on main
. that being said, I don't think we really need this triggered on PR activity? I imagine most, if not all of these use cases would be caught by on: push
. Additionally, can we add a workflow_dispatch:
trigger here in case we want to manually trigger this deployment for whatever reason?
@nutjob4life has become our GitHub Actions guru as well, so maybe he has some thoughts?
🗒️ Summary
Deployment of registry-api (on JPL cloud) and nucleus (on NGAP) completed. Created new terraform cicd workflow for nucleus, combining TF plan and apply.
⚙️ Test Data and/or Report
One of the following should be included here:
♻️ Related Issues
NASA-PDS/operations#367