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

feat: Add service_directory_registrations: namespace and service_directory_region #548

Conversation

sanadhis
Copy link
Contributor

Hi, I would propose to add optional variable of project_region for this module.

The idea is to allow specifying custom, not in us-central1, namespace link.

@sanadhis sanadhis requested review from imrannayer and a team as code owners February 19, 2024 13:10
@imrannayer
Copy link
Collaborator

@sanadhis thx for the PR. Can you plz add two optional variables

service_directory_region
service_directory_namespace

and use dynamic blocks if any of these values are provided.

@sanadhis sanadhis force-pushed the feat-project-region-for-private-service-connect branch from 022a0eb to e12768e Compare February 20, 2024 18:41
@sanadhis sanadhis requested a review from imrannayer February 21, 2024 08:28
@sanadhis
Copy link
Contributor Author

Hi @imrannayer thanks for the feedback!

I've addressed them, can you re-review? Thanks 😄

@sanadhis
Copy link
Contributor Author

Hi @imrannayer can we merge here if it's all okay? thanks 😄

@imrannayer
Copy link
Collaborator

@sanadhis Lint test is failing. Can u plz fix it?

@imrannayer
Copy link
Collaborator

@sanadhis can u plz follow this doc to fix the linit issues?

@sanadhis sanadhis force-pushed the feat-project-region-for-private-service-connect branch from c002cc6 to 67ca7ca Compare February 28, 2024 20:18
@sanadhis
Copy link
Contributor Author

sanadhis commented Feb 28, 2024

@sanadhis can u plz follow this doc to fix the linit issues?

thanks for the hint @imrannayer
Lint test passes in my machine now:

...
terraform_validate ./test/fixtures/simple_project_with_regional_network
Success! The configuration is valid.

terraform_validate ./test/fixtures/submodule_firewall
Success! The configuration is valid.

terraform_validate ./test/setup
Success! The configuration is valid.

ENABLE_BPMETADATA not set to 1. Skipping metadata validation.

@imrannayer
Copy link
Collaborator

/gcbrun

@imrannayer imrannayer changed the title feat: allow to specify project_region for private-service-connect feat: Add service_directory_registrations: namespace and service_directory_region Feb 29, 2024
@imrannayer
Copy link
Collaborator

/gcbrun

@imrannayer
Copy link
Collaborator

imrannayer commented Feb 29, 2024

@sanadhis this feature was part of 5.8. Can you plz update version.tf file to use provider version >= 5.8?
This will make it a breaking change. Can you also add a file with details in docs directory.

Thanks

@imrannayer
Copy link
Collaborator

/gcbrun

1 similar comment
@imrannayer
Copy link
Collaborator

/gcbrun

@sanadhis sanadhis force-pushed the feat-project-region-for-private-service-connect branch from 871d03e to 688f678 Compare March 1, 2024 06:55
@sanadhis
Copy link
Contributor Author

sanadhis commented Mar 1, 2024

@imrannayer should I update modules/private-service-connect/metadata.yaml and provider_meta in modules/private-service-connect/versions.tf or do you have automation when releasing this module?

@imrannayer
Copy link
Collaborator

@sanadhis thats not needed. version meta will update by release scripts

@imrannayer
Copy link
Collaborator

/gcbrun

@imrannayer imrannayer self-requested a review March 4, 2024 16:27
@imrannayer imrannayer merged commit d4855d6 into terraform-google-modules:master Mar 4, 2024
4 checks passed
@stanimprover
Copy link

@sanadhis @imrannayer I'm getting an Unsupported block type Error for the dynamic "service_directory_registrations" for the private-service-connect module and I have changed to multiple versions and still not working. Are you having this issue as well?

@imrannayer
Copy link
Collaborator

@stanimprover can u plz create an issue. This is part of 9.1 which is not released yet. Can you also post the code you are using for your testing?

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

Successfully merging this pull request may close these issues.

3 participants