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: Set edge_security_policy optional and add session_affinity in variables #333

Merged
merged 4 commits into from
May 24, 2023
Merged

Conversation

imrannayer
Copy link
Collaborator

@imrannayer imrannayer commented May 20, 2023

fix #330
fix #331
fix #332
fix #323
fix #324

@imrannayer imrannayer requested a review from a team as a code owner May 20, 2023 01:16
Copy link
Contributor

@g-awmalik g-awmalik left a comment

Choose a reason for hiding this comment

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

Looks fine for the most part but I see a lot of comments in the example files. Not sure if that's intentional. If so, can you explain why?

@@ -85,65 +85,65 @@ variable "backends" {
description = "Map backend indices to list of backend maps."
type = map(object({
{% if not serverless %}{# not necessary for serverless as default port_name=http, protocol=HTTP #}
Copy link
Contributor

Choose a reason for hiding this comment

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

If the fields are all optional now, do we need this template check? Does it matter if this var exists in all modules generated for this template?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although fields are optional now but I am worried people will provide value and get error. In order to avoid it I just wanted to remove it from Serverless NEG. Since these are all optional people can use main module to deploy serverless NEG.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair. I would suggest tracking that as an issue, if not already present.

examples/cdn-policy/main.tf.old Outdated Show resolved Hide resolved
examples/cloudrun/main.tf Outdated Show resolved Hide resolved
examples/cloudrun/main.tf Outdated Show resolved Hide resolved
examples/cloudrun/main.tf Outdated Show resolved Hide resolved
@imrannayer imrannayer merged commit 853ccba into terraform-google-modules:master May 24, 2023
@imrannayer imrannayer self-assigned this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment