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

Fix: Update tf vars #147

Merged
merged 3 commits into from
Jul 11, 2023
Merged

Fix: Update tf vars #147

merged 3 commits into from
Jul 11, 2023

Conversation

sailorlqh
Copy link
Contributor

@sailorlqh sailorlqh commented Jul 11, 2023

I think it is a better way to have defaultResourceScope set only for mapping service.

By doing this, PMAP_MAPPING_DEFAULT_RESOURCE_SCOPE won't appear in policy service's env vars, and we don't need to default var.pmap_mapping_default_resource_scope and parse it to project/project-id for mapping service.

Also, using extra_container_env_vars (maybe a better naming can be discussed later), we can customize different env vars for different services.

There are some other cases for using merge() to setup env vars in terraform-module repo. So I think this might be a good approach.

I want to get some input from you before I convert this draft PR into ready for review.

@sailorlqh sailorlqh force-pushed the sailorlqh/update_service_tf branch from edfa649 to 2f2cf8a Compare July 11, 2023 20:47
@sailorlqh sailorlqh force-pushed the sailorlqh/update_service_tf branch from b7b0abe to a1c974e Compare July 11, 2023 21:36
@sailorlqh sailorlqh marked this pull request as ready for review July 11, 2023 21:49
@sailorlqh sailorlqh requested a review from a team as a code owner July 11, 2023 21:49
@sailorlqh
Copy link
Contributor Author

/Ready for review.

@sailorlqh sailorlqh force-pushed the sailorlqh/update_service_tf branch from a1c974e to b607d12 Compare July 11, 2023 22:04
@sailorlqh sailorlqh force-pushed the sailorlqh/update_service_tf branch from b607d12 to 146aa7e Compare July 11, 2023 22:04
.github/workflows/ci.yml Outdated Show resolved Hide resolved
terraform/e2e/main.tf Outdated Show resolved Hide resolved
terraform/e2e/variables.tf Outdated Show resolved Hide resolved
terraform/e2e/variables.tf Outdated Show resolved Hide resolved
terraform/e2e/variables.tf Outdated Show resolved Hide resolved
terraform/modules/pmap-service/variables.tf Outdated Show resolved Hide resolved
terraform/modules/pmap-service/variables.tf Outdated Show resolved Hide resolved
@sailorlqh sailorlqh merged commit 85f9b1c into main Jul 11, 2023
@sailorlqh sailorlqh deleted the sailorlqh/update_service_tf branch July 11, 2023 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants