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

Rodriguezsergio/fix https proxy resource #2779

Conversation

rodriguezsergio
Copy link
Contributor

@rodriguezsergio rodriguezsergio commented Dec 19, 2024

My local copy of this repo at this commit fails during a Terraform plan when I define a value for https_proxy_config.certificate_manager_certificates.

This PR fixes that in not only net-lb-app-int, but a few other directories.

I did NOT run e2e tests. I looped through the affected directories and ran the following

pytest -k 'modules and $target_dir: and not e2e'

Checklist

If 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

@rodriguezsergio rodriguezsergio marked this pull request as draft December 19, 2024 19:22
@ludoo
Copy link
Collaborator

ludoo commented Dec 19, 2024

Can you please provide a repro of what exactly is breaking, and the error message?

@ludoo
Copy link
Collaborator

ludoo commented Dec 19, 2024

And did you check if this is merged in your branch?

#2764

@rodriguezsergio rodriguezsergio force-pushed the rodriguezsergio/fix-https-proxy-resource branch from d15c08b to 01e34c3 Compare December 19, 2024 22:26
@rodriguezsergio
Copy link
Contributor Author

@ludoo it looks like someone beat me to it for that single directory. I will rebase against main and adopt that style change across all net-lb- directories.

My first attempt was here #2665 but I got caught up with my day job. I'm in the process of verifying that all tests pass now (e2e tests excluded).

Are non-google contributors expected to run e2e tests?

@rodriguezsergio rodriguezsergio force-pushed the rodriguezsergio/fix-https-proxy-resource branch from e4a8579 to ff72c38 Compare December 19, 2024 23:00
@@ -215,7 +215,6 @@ values:
description: Terraform managed.
enable_cdn: null
failover_policy: []
iap: []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error kept popping up for me. If this is not the right way to address this, please let me know.

================================================================= short test summary info =================================================================
FAILED tests/examples/test_plan.py::test_example[terraform:modules/net-lb-ext:Referencing existing MIGs:1] - AssertionError: tests/modules/net_lb_ext/examples/migs.yaml: .module.nlb.google_compute_region_backend_service.default.iap is not a valid address in t...
FAILED tests/examples/test_plan.py::test_example[terraform:modules/net-lb-ext:Externally managed instances:1] - AssertionError: tests/modules/net_lb_ext/examples/ext_migs.yaml: .module.nlb.google_compute_region_backend_service.default.iap is not a valid address ...
FAILED tests/examples/test_plan.py::test_example[terraform:modules/net-lb-ext:Multiple forwarding rules:1] - AssertionError: tests/modules/net_lb_ext/examples/fwd_rules.yaml: .module.nlb.google_compute_region_backend_service.default.iap is not a valid address...
FAILED tests/examples/test_plan.py::test_example[terraform:modules/net-lb-ext:Dual stack (IPv4 and IPv6):1] - AssertionError: tests/modules/net_lb_ext/examples/dual_stack.yaml: .module.nlb.google_compute_region_backend_service.default.iap is not a valid addres...
FAILED tests/examples/test_plan.py::test_example[terraform:modules/net-lb-ext:End to end example:1] - AssertionError: tests/modules/net_lb_ext/examples/e2e.yaml: .module.nlb.google_compute_region_backend_service.default.iap is not a valid address in th...
====================================================== 5 failed, 978 deselected in 113.49s (0:01:53) ======================================================

@rodriguezsergio rodriguezsergio force-pushed the rodriguezsergio/fix-https-proxy-resource branch from df72d50 to 4deff4f Compare December 20, 2024 03:46
@@ -31,8 +31,7 @@ variable "backend_service_configs" {
session_affinity = optional(string)
timeout_sec = optional(number)
backends = list(object({
# group renamed to backend
backend = string
group = string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not change this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The names are not consistent across modules.

There are missing properties across modules as well. Can you elaborate why standardizing on 1 name is not advised?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Some properties are missing because different load balancera support different features.

I would not touch those modules, they are very complex and delicate and have worked well for a long time.

@ludoo
Copy link
Collaborator

ludoo commented Dec 20, 2024

I am closing this, if you reopen can you please

  • clearly state what you are fixing, showing code for repro and error messages?
  • tackle one issue at a time so that reviewers can figure out what the PR is solving?

Thanks :)

@ludoo ludoo closed this Dec 20, 2024
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.

2 participants