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

Implemented conditional dynamic blocks for google_access_context_manager_service_perimeter spec and status #1177

Merged

Conversation

calexandre
Copy link
Contributor

@calexandre calexandre commented Feb 24, 2023

This PR implements conditional dynamic blocks for google_access_context_manager_service_perimeter spec and status blocks.

This way, ensures that spec and status blocks are only created when there is a spec or status respectively.

@google-cla
Copy link

google-cla bot commented Feb 24, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

juliocc
juliocc previously approved these changes Feb 24, 2023
@juliocc juliocc enabled auto-merge February 24, 2023 16:25
@juliocc juliocc disabled auto-merge February 24, 2023 16:28
@calexandre
Copy link
Contributor Author

calexandre commented Feb 24, 2023

@juliocc I'm gonna need some help on this one...I've gone through the logs, and I think this is failing due to the current test plan which is trying to create a case where it omits both the spec and status blocks leading to the given errors.

I can help fix the tests, but I will need some guidance on that one...

What are your thoughts?

@juliocc
Copy link
Collaborator

juliocc commented Feb 24, 2023

So this PR is actually breaking tests. I'll leave it up to @ludoo since that's his stuff

@ludoo
Copy link
Collaborator

ludoo commented Feb 24, 2023

Hey Carlos, if you're not in too much of a hurry I can get to it this weekend. The change will probably impact the FAST security stage too, and I need to check it's not breaking stuff.

@ludoo ludoo self-assigned this Feb 24, 2023
@juliocc juliocc dismissed their stale review February 24, 2023 17:05

Not as simple as it seemed

@ludoo ludoo self-requested a review February 24, 2023 17:05
@calexandre
Copy link
Contributor Author

Hey Carlos, if you're not in too much of a hurry I can get to it this weekend. The change will probably impact the FAST security stage too, and I need to check it's not breaking stuff.

Sure thing. Let me know if you need anything else.

@ludoo
Copy link
Collaborator

ludoo commented Feb 25, 2023

Ok, so my reasoning is that you can only do that for spec, as the status block needs to be there even with no resources if you set use_explicit_dry_run_spec. If this is ok with you, I can merge this.

@calexandre
Copy link
Contributor Author

Ok, so my reasoning is that you can only do that for spec, as the status block needs to be there even with no resources if you set use_explicit_dry_run_spec. If this is ok with you, I can merge this.

Sounds like a plan! Let's go with your suggestion 👍

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.

Thanks a lot for spotting this, and your patience while we checked the test errors! :)

@ludoo ludoo enabled auto-merge (squash) February 25, 2023 15:49
@ludoo ludoo merged commit aecb6fd into GoogleCloudPlatform:master Feb 25, 2023
@calexandre calexandre deleted the fix/perimeter-dynamic-blocks branch February 27, 2023 14:08
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.

Permadiff in google_access_context_manager_service_perimeter
3 participants