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.

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

Add Alerts, Logging, Channels Factories #2758

Merged
merged 45 commits into from
Jan 5, 2025

Conversation

joshw123
Copy link
Contributor

@joshw123 joshw123 commented Dec 10, 2024


Checklist

I applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

@joshw123 this is great stuff!

I left comments below, but besides that:

  1. We need to update project/README.md to include examples the new variables and factories
  2. FAST tests need to be updated

I have a few more things, but let's start with this batch.

modules/project/variables.tf Outdated Show resolved Hide resolved
modules/project/alerts-factory.tf Outdated Show resolved Hide resolved
modules/project/README.md Outdated Show resolved Hide resolved
modules/project/alerts-factory.tf Outdated Show resolved Hide resolved
modules/project/logging-metrics-factory.tf Outdated Show resolved Hide resolved
modules/project/variables-metrics-alerts.tf Outdated Show resolved Hide resolved
modules/project/variables-metrics-alerts.tf Outdated Show resolved Hide resolved
modules/project/variables-metrics-alerts.tf Outdated Show resolved Hide resolved
modules/project/variables-metrics-alerts.tf Outdated Show resolved Hide resolved
modules/project/variables-metrics-alerts.tf Outdated Show resolved Hide resolved
@joshw123
Copy link
Contributor Author

@joshw123 this is great stuff!

I left comments below, but besides that:

  1. We need to update project/README.md to include examples the new variables and factories
  2. FAST tests need to be updated

I have a few more things, but let's start with this batch.

Thank You, Ill look at the above, Im checking through all the fields too to ensure optional and required match the docs, and to include validation where necessary :)

@joshw123
Copy link
Contributor Author

@ludoo @juliocc The only two tests that are still not passing are Module Examples Test report

examples.test_plan.test_example[terraform:modules/project-factory:Example:1]
AssertionError: wrong number of resources
assert 56 == 59

examples.test_plan.test_example[terraform:modules/project-factory:Tests:1]
AssertionError: wrong number of resources
assert 22 == 25

I don't know if its just me, but I can't seem to see where it is getting them values, could you point me in the right direction please? :)

Thank You.

@juliocc
Copy link
Collaborator

juliocc commented Dec 13, 2024

examples.test_plan.test_example[terraform:modules/project-factory:Example:1] AssertionError: wrong number of resources assert 56 == 59

Look at the #tftest tag at the end of the code block in this section.

examples.test_plan.test_example[terraform:modules/project-factory:Tests:1] AssertionError: wrong number of resources assert 22 == 25

Same thing here.

Take a look at the contributing guide for more details.

@joshw123 joshw123 requested review from juliocc and ludoo December 13, 2024 15:28
@ludoo
Copy link
Collaborator

ludoo commented Dec 15, 2024

Can you sign the CLA from the ju****c​@gmail.com account?

@juliocc
Copy link
Collaborator

juliocc commented Dec 15, 2024

Can you sign the CLA from the ju****c​@gmail.com account?

That address is mine. Should be fixed now.

@juliocc
Copy link
Collaborator

juliocc commented Dec 16, 2024

@joshw123 I'll do my second pass today. Apologies for the delay.

Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

Leaving a few more comments below.

I also sent you a PR against your branch with a few styling fixes. Once you get that merged I'll try to get started on removing the default_alerts_email variable and use essential_contacts

fast/stages/0-bootstrap/outputs.tf Outdated Show resolved Hide resolved
fast/stages/0-bootstrap/variables.tf Outdated Show resolved Hide resolved
fast/stages/0-bootstrap/billing.tf Outdated Show resolved Hide resolved
modules/project/alerts.tf Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the on:FAST label Jan 5, 2025
@juliocc
Copy link
Collaborator

juliocc commented Jan 5, 2025

I updated this PR to change only modules/project and modueles/project-factory. I'll send a separate PR for FAST

@juliocc juliocc requested a review from ludoo January 5, 2025 18:43
Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

Awesome stuff. Can we wait a couple of days for fast and do it in fast dev, as I'm almost ready to send a large pr there...

@juliocc
Copy link
Collaborator

juliocc commented Jan 5, 2025

Awesome stuff. Can we wait a couple of days for fast and do it in fast dev, as I'm almost ready to send a large pr there...

Absolutely. That's why I removed the fast part.

@juliocc juliocc enabled auto-merge (squash) January 5, 2025 19:10
@juliocc juliocc disabled auto-merge January 5, 2025 19:10
@juliocc juliocc enabled auto-merge (squash) January 5, 2025 19:10
@juliocc juliocc merged commit 325a997 into GoogleCloudPlatform:master Jan 5, 2025
14 checks passed
@joshw123
Copy link
Contributor Author

joshw123 commented Jan 5, 2025

Thank You for finishing this @juliocc @ludoo Apologies for being absent, I have been in hospital with Flu and with Christmas and everything totally forgot, thank you again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants