-
Notifications
You must be signed in to change notification settings - Fork 1
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
NAP-360 #110
NAP-360 #110
Conversation
Hey @chris-bridgett-nandos, IMHO nothing should change so I find
I think I would do a dry run in an infra-dev project to understand what is going on. |
Also before we do this we need to back up the state file and the BigQuery data. |
So:
To dissect what is happening a little more:
I believe these changes are okay?
Correct. Most of the Terraform code that was previously in
I don't think Totally onboard re. not losing data. Happy to throw it in an infra-dev and/or backup our BQ dataset when the time comes to merge. In fact, will update the original PR comment with that now. |
Okay. With the secret value change? |
I don't think so. In fact, going to hop on NAP-371 now and resolve that, that should make things clearer. |
New Cloud Build log post-NAP-371. The only part that I'm not 100% on is why it's recreating IAM for the secret, but it doesn't overly concern me. Also raised an infrastructure PR here in order to deploy this stuff in an isolated environment. |
@matthew-green-nandos ready for review (cannot re-request, since you didn't request changes before). |
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.
Some change to just tidy up the code and to make sure PagerDuty keeps working.
I find the code base confusing with Terraform being in experimental and setup. Also the mixing of languages. I am not sure I understand it works as it seems all over the place. IMHO there should be experimental branches rather than folders.
experimental/terraform/modules/fourkeys-dashboard/files/dashboard/grafana.ini
Outdated
Show resolved
Hide resolved
Sorted. 👌
As far as I'm aware, they represent different approaches. Nothing from
Mixing of languages? Sorry, I'm not sure I follow unless you're referring to HCL/bash/python? (HCL for infrastructure, bash for bootstrapping, python for the application?)
I don't disagree! This is the maintainer's approach at the moment. I suppose they may originally have wanted to offer visibility of their preliminary terraform approach or other experimental features to start with, without hiding it away in a branch. 🤷 |
@chris-bridgett-nandos some more thoughts:
Should all of our code be in the setup folder then (I might be missing the trees for the wood/forest of changes)?
Yes I was referring to HCL/bash/python |
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.
Let's go for it.
Yep, and I believe it is. 🙂 Upstream still maintains some experimental things in the experimental directory, which are of no bearing to us (our triggers now point at the setup directory[1]). [1] ...or at least they will once this PR is merged. Almost forgot. |
Overview:
setup.sh
, but we were already usingexperimental/terraform
. Changes in infrastructure have already been made to point at the correctcloudbuild.yaml
.grafana
, but our trigger passes inlooker
(already done ininfrastructure
).The(new plan further down). Things are mostly just updated in place due to some extra meta.terraform plan
looks okay to meOverviewed Continued 17/03/22:
events_raw
from the fourkeys project and generated data. ✅setup/README.md
for provisioning fourkeys from scratch via Terraform).terraform plan
for review.Tasks:
events_raw
Big Query dataset