-
Notifications
You must be signed in to change notification settings - Fork 95
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: added v2 service as submodule #200
Conversation
/gcbrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/gcbrun |
@@ -0,0 +1,86 @@ | |||
# Cloud Run v2 Service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should a metadata.yaml file be created for this new module? I don't see metadata.yaml available for this module. But we do have this file for modules present in https://github.com/terraform-google-modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very sure of a metadata.yaml files usage. @bharathkkb Should I generate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prabhu34 you can set this in makefile to autogenerate https://github.com/terraform-google-modules/terraform-google-folders/blob/696570e8cf5d996336b586c9293ae6fea3edf98c/Makefile#L79
go/blueprints-metadata has more instructions but feel free to do this in a follow up to not block this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created the metadata now. Seems like the Makefile was outdated and I had added the env to the generate_docs.
@q2w Feel free to make changes and amend! |
/gcbrun |
/gcbrun |
@bharathkkb For your second review with changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! A few minor comments otherwise lgtm
dynamic "containers" { | ||
for_each = var.containers | ||
content { | ||
name = containers.value.container_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use key rather than using a separate container_name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that earlier, but having a container name is optional input and container image will be a bigger key name. Let me know your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it list(objects) instead.
tag = traffic.value.tag | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to ignore chnage any annotations added by the service like
terraform-google-cloud-run/main.tf
Line 192 in b59b700
ignore_changes = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these are not needed as they will be part of effective_annotations and we can add them later if really needed.
/gcbrun |
/gcbrun |
cloud_run_v2_service
submodule@bharathkkb @q2w