-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
cleanup: add region tags to yaml and sh #684
Conversation
Here is the summary of changes. You are about to add 12 region tags.
This comment is generated by snippet-bot.
|
I had tags generated for the |
|
🚲 PR staged at http://34.71.99.106 |
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.
Thanks for creating this pull-request, @ckim328! :)
And great work.
I left a few comments.
One question is about the need for this change.
Let's address that before the other comments. :)
kubernetes-manifests/adservice.yaml
Outdated
@@ -12,6 +12,7 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
# [START gke_kubernetes_manifests_adservice_deployment_adservice] |
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.
Issue:
I noticed that we're adding region tags to file YAML files in kubernetes-manifests/
. All these YAML files get concatenated into one file, release/kubernetes-manifests.yaml
, when we create a new release.
So, for instance, this
# [START gke_kubernetes_manifests_adservice_deployment_adservice]
region tag would exist here and in the release/kubernetes-manifests.yaml
file.
Will this create any problems (having a region tag show up in two places)?
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.
ooooh good point.
this will cause a problem, since we shouldn't have duplicate region tags (with the same language, in this case yaml)
I'll try to see a workaround, thanks for bringing this up!!
I think the make-release-artifacts.sh can be edited to remove the tags (I don't think we need to have them in the release manifests)
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 agree. Adding code to remove lines look like # [START ... ]
and # [END ...
in make-release-artifacts.sh
might be the best solution.
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.
Addressed this! I added 2 lines for the kubernetes and istio manifest section.
For example here
sed -i '' '/[^# \[]]/d' "${k8s_manifests_file}"
finds lines that start with # [
and deletes it
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.
Awesome! Thanks so much for adding this in. :)
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.
after discussion with Megan, I edited make-release-artifacts.sh to add region tags around the release/kubernetes-manifests.yaml
and the release/istio-manifests.yaml
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
|
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
1 similar comment
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
🚲 PR staged at http://34.71.99.106 |
1 similar comment
🚲 PR staged at http://34.71.99.106 |
This PR is ready to review + merge! Since there were no region tags to begin with in |
🚲 PR staged at http://34.71.99.106 |
* cleanup: add region tags to yaml and sh (GoogleCloudPlatform#684) * Added region tags to yaml and sh, edited the asm and gke files * Fix skaffold artifacts * make-release adds region tag to istio and k8 manifest * Addressed comments, changed gke to container per discussion * Replaced instances of container with gke, discarded uncessecary white space changes * Remove changes in github workflow, captured cloudbuild yaml tag * Bump node-fetch from 2.6.6 to 2.6.7 in /src/paymentservice (GoogleCloudPlatform#707) Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.6 to 2.6.7. - [Release notes](https://github.com/node-fetch/node-fetch/releases) - [Commits](node-fetch/node-fetch@v2.6.6...v2.6.7) --- updated-dependencies: - dependency-name: node-fetch dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump node-fetch from 2.6.6 to 2.6.7 in /src/currencyservice (GoogleCloudPlatform#708) Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.6 to 2.6.7. - [Release notes](https://github.com/node-fetch/node-fetch/releases) - [Commits](node-fetch/node-fetch@v2.6.6...v2.6.7) --- updated-dependencies: - dependency-name: node-fetch dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Shabir Mohamed Abdul Samadh <[email protected]> Co-authored-by: Christine Kim <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Shabir Mohamed Abdul Samadh <[email protected]>
Background
Adding region tags to our yaml and shell files. Allows for our team to leverage internal tools!
Fixes
n/a
Change Summary
Added region tags. Snippet-bot will add a comment below of the tags added (and where).
Tags are
gke_...
andservicemesh_...
Additional Notes
Since there were no existing region tags in this repo, we don't have to worry about any internal references to tags (ie no existing code widgets!)
Also please note that this update will take a couple hours or more to reflect internally.
Testing Procedure
n/a
Related PRs or Issues
n/a