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

fixup(project-factory): Use the correct KMS Service Agents attribute … #1446

Merged
merged 4 commits into from
Jun 19, 2023
Merged

fixup(project-factory): Use the correct KMS Service Agents attribute … #1446

merged 4 commits into from
Jun 19, 2023

Conversation

alloveras
Copy link
Contributor

Intent

To fix a bug affecting the project-factory blueprint that was preventing the kms_service_agents entries in **/*.yaml from being propagated into the project blueprint module.

Problem

The file fast/stages/3-project-factory/dev/main.tf seems to expect the YAML key to be named kms instead of kms_service_agents. The key lookup for that attribute is wrapped in a try() clause which causes the naming mismatch to go unnoticed because the execution always falls back to the {} default value and, thus, any values for kms_service_agents are being discarded.

Solution

To update the fast/stages/3-project-factory/dev/main.tf and README.md files to use the correct attribute name for KMS Service Agents configurations.

@ludoo ludoo enabled auto-merge (squash) June 19, 2023 06:24
auto-merge was automatically disabled June 19, 2023 06:41

Head branch was pushed to by a user without write access

@alloveras
Copy link
Contributor Author

alloveras commented Jun 19, 2023

Hey @ludoo,
can you provide some guidance on how to get the unit tests passing again? I tried to get them working by adding the new resources that will be created now that KMS Service Agents are handled properly but, for some reason, I am still getting test failures reporting a mismatch between expected and actual resource counts 😢

In fact, if I run the following locally it seems like they pass?

$ pytest -vv -n4 -k blueprints/ tests/examples
==================================================================================================================================================== test session starts =====================================================================================================================================================
platform darwin -- Python 3.11.2, pytest-7.3.2, pluggy-1.0.0 -- /Users/alloveras/.venv/gcp/bin/python3.11
cachedir: .pytest_cache
rootdir: /Users/alloveras/work/gcp-cloud-foundation-fabric
plugins: xdist-3.3.1
4 workers [57 items]
scheduling tests via LoadScheduling
....
[gw2] [ 52%] PASSED tests/examples/test_plan.py::test_example[blueprints/factories/project-factory:Terraform code]
....
=============================================================================================================================================== 57 passed in 340.15s (0:05:40) ===============================================================================================================================================

Thank you in advance!

@juliocc
Copy link
Collaborator

juliocc commented Jun 19, 2023

Just change this line to reflect resources=34

As for the local tests, you're not selecting the right tests. Try with pytest -vv -k 'blueprints and project-factory'

@alloveras
Copy link
Contributor Author

Just change this line to reflect resources=34

As for the local tests, you're not selecting the right tests. Try with pytest -vv -k 'blueprints and project-factory'

Right, thank you for the pointer 🙇 Now I wonder if the changes that I did before to //tests/blueprints/factories/project_factory/examples/example.yaml are actually required?

@ludoo ludoo enabled auto-merge (squash) June 19, 2023 23:35
@ludoo ludoo merged commit 7cacc46 into GoogleCloudPlatform:master Jun 19, 2023
@alloveras alloveras deleted the alloveras-fixup-project-factory-kms-attribute branch June 20, 2023 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants