Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Kustomize variation to deploy Online Boutique apps without the
grpc-health-probe
bin #1102Kustomize variation to deploy Online Boutique apps without the
grpc-health-probe
bin #1102Changes from 30 commits
f3e6ef7
b1e0835
b53a97b
f458993
8d1bd03
9c31b94
921a04c
1f4d261
c146e00
e44a138
bd38492
72a2bfb
5ed543a
0ca4a52
77a38a0
403eb0b
7c5acf7
722907a
a93644a
bf130b4
37c0688
13d7c01
9fde09b
d05e994
8908fec
7b3096d
3e0e2c3
002c0db
4c227ff
f4f3360
1941536
a7d3fc7
533fdab
3bfd68d
15dd029
a92c135
f60a67f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Rationale for my change:
I've changed
"s/CONTAINER_IMAGES_REGISTRY/$REGISTRY/g"
to"s|CONTAINER_IMAGES_REGISTRY|$REGISTRY|g"
because the/
s insidegcr.io/my-project/my-directory
were breaking thesed
command. Now we're no longer using regexes (we're using|
s instead) to avoid/
s from the${REGISTRY}
variable being interpreted as special characters.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.
Suggestion: Should we update this
sed
command?Regarding: #1102 (comment)
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.
Oh yeah, maybe a merge issue, let me check that and update this.
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.
Question: Is there a reason we remove the curly braces?
Should we also fix:
inside
kustomize/components/memorystore/README.md
?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.
Maybe it was an accident?
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.
Hey Nim, good question, actually I had that in the description:
Review the setup of tokens to avoid issues with Kustomize and bash session
. So that's on purpose otherwise we have issue in bash session when doing thesed
part and/or when doingkustomize build .
without these tokens replaced. Let me know if you need more info. But the tl,dr is we should have token just like thisREDIS_ADDR
not with any{}
nor{{}}
, etc.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.
Following this today, one possible unintended side effect is that the
sed
now matches the name and value. So I end up with: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.
Yep, that's why we document this like this: f60a67f. But yeah if someone is not doing the
\"
it could land to this error. Open for suggestions to improve this for the different files having tokens like this. PR about that are more than welcome ;)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.
Correct. I checked that I pulled latest and just reran the terraform deploy. I see the escape characters and the quote
\"
and it still seems to be replacing all of them.To clarify, it worked in the sense that the application deployed beautifully, it just replaced all instances of
REDIS_ADDR
with10.34.24.75:6379
. (Including the instance at the top of the/microservices-demo/kustomize/components/memorystore/kustomization.yaml
file.)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.
Gotcha, thanks. Do you mind reporting this as a dedicated issue please? We will need to tackle this to fix this broken Terraform deployment with Memorystore. Thanks!
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'll do that!
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.
FYI: this will be fixed via #1202. Thanks!
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.
Nice!
I also had a solution (all the backslashes!!) that I was going to PR today, but that is much cleaner.