-
Notifications
You must be signed in to change notification settings - Fork 11
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
Update terraform and workflow to deploy Reporting V2 #1223
Update terraform and workflow to deploy Reporting V2 #1223
Conversation
64b44de
to
92aa7f8
Compare
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.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tristanvuong2021)
48cfa61
to
ca20947
Compare
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.
Reviewed 1 of 1 files at r2, 6 of 6 files at r3, 2 of 2 files at r4.
Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @tristanvuong2021)
.github/workflows/configure-reporting-v2.yml
line 89 at r4 (raw file):
run: > bazelisk build "//src/main/k8s/dev:reporting_v2.tar"
nit: why is this in quotes?
.github/workflows/terraform-cmms.yml
line 99 at r4 (raw file):
working-directory: ${{ env.GCLOUD_MODULE_PATH }} run: terraform apply -input=false tfplan
Why was this all removed? This is needed for the worker2 Duchy which runs in AWS.
src/main/terraform/gcloud/cmms/reporting_v2.tf
line 15 at r4 (raw file):
# limitations under the License. module "reporting_cluster" {
These names all need to be changed I think. Terraform basically just concats all the .tf files together.
Code quote:
"reporting_cluster"
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.
Reviewable status: 6 of 10 files reviewed, 3 unresolved discussions (waiting on @Marco-Premier and @SanjayVas)
.github/workflows/configure-reporting-v2.yml
line 89 at r4 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
nit: why is this in quotes?
I copied it from the other file. I removed quotes from both
.github/workflows/terraform-cmms.yml
line 99 at r4 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Why was this all removed? This is needed for the worker2 Duchy which runs in AWS.
I removed it to try to make the testing run faster.
src/main/terraform/gcloud/cmms/reporting_v2.tf
line 15 at r4 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
These names all need to be changed I think. Terraform basically just concats all the .tf files together.
Done.
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.
Reviewed 4 of 5 files at r6, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)
.github/workflows/terraform-cmms.yml
line 99 at r4 (raw file):
Previously, tristanvuong2021 (Tristan Vuong) wrote…
I removed it to try to make the testing run faster.
Leave it commented out with a DO_NOT_SUBMIT comment then so it's clear that you intend to put it back before submitting.
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.
Reviewable status: 4 of 16 files reviewed, 1 unresolved discussion (waiting on @Marco-Premier and @SanjayVas)
.github/workflows/terraform-cmms.yml
line 99 at r4 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Leave it commented out with a DO_NOT_SUBMIT comment then so it's clear that you intend to put it back before submitting.
Done.
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.
Reviewed 8 of 11 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)
src/main/k8s/reporting_v2.cue
line 21 at r8 (raw file):
_verboseGrpcClientLogging: bool | *false _reportSchedulingCronSchedule: "30 6 * * *" // Daily at 6:30 AM
Shouldn't this be overridable?
Suggestion:
_reportSchedulingCronSchedule: string | *"30 6 * * *" // Daily at 6:30 AM
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.
Reviewable status: 15 of 16 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/k8s/reporting_v2.cue
line 21 at r8 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Shouldn't this be overridable?
Done.
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.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tristanvuong2021)
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.
Reviewed 1 of 1 files at r2, 1 of 6 files at r3, 2 of 5 files at r6, 7 of 11 files at r7, 4 of 4 files at r8, 1 of 1 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tristanvuong2021)
There will be one cluster for Reporting V1 and one cluster for Reporting V2.