-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Refactor Kustomize components (service-accounts, shopping-assistant) #2488
Conversation
🚲 PR staged at http://104.154.159.254 |
🚲 PR staged at http://104.154.159.254 |
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.
It looks good but I think we can improve the sed experience with a kustomize overflow.
sed -i "s/ALLOYDB_DATABASE_NAME_VAL/${ALLOYDB_PRODUCTS_DATABASE_NAME}/g" kubernetes-manifests/shoppingassistantservice.yaml | ||
sed -i "s/ALLOYDB_TABLE_NAME_VAL/${ALLOYDB_PRODUCTS_TABLE_NAME}/g" kubernetes-manifests/shoppingassistantservice.yaml | ||
sed -i "s/ALLOYDB_SECRET_NAME_VAL/${ALLOYDB_SECRET_NAME}/g" kubernetes-manifests/shoppingassistantservice.yaml | ||
# Substitute environment values (kustomize/components/shopping-assistant/shoppingassistantservice.yaml) |
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.
Instead all these seds should we just add a overlay for these values?
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.
The vars are in overlays already. Sed is just doing the var substitution with values.
If we have another overlays explicitly just for variables, then we still have to sed that overlay so we're back at square one. We can't hardcode those values in the overlay because they're dynamic, like the name of the GSA or the Google Cloud project ID.
Either way I spin it in my head, we need to either:
a) sed something; or
b) ask the user to manually replace vars somewhere (which is more friction than sed)
Unless I'm misunderstanding what you mean?
/lgtm |
This PR:
Note that this means the shopping assistant is no longer with the other services, but rather as part of a separate component.
Fixes #2468
Testing
I tested this by deploying with the following two components uncommented:
Which deployed successfully with a working shopping assistant. And then I deployed with the two components commented out (the default state) which successfully deployed without the shopping assistant, as intended: