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

Move YAML document separator to top of release manifests #799

Merged
merged 1 commit into from
Apr 12, 2022
Merged

Move YAML document separator to top of release manifests #799

merged 1 commit into from
Apr 12, 2022

Conversation

dsbibby
Copy link
Contributor

@dsbibby dsbibby commented Apr 7, 2022

Background

I propose moving the YAML document separator inserted as part of make-release-artifacts.sh to the top of each manifest rather than the bottom.

Having the separator at the end of the manifest results in the generated release YAML files containing an empty document at the end. Whilst this is valid YAML, it is untidy, and certain YAML tools (yq) have been seen to handle empty document elements badly.

Fixes

Change Summary

Moved the YAML separator --- to before each service manifest rather than after

Additional Notes

Testing Procedure

Tested and deployed generated manifests succesfully

Related PRs or Issues

I propose moving the YAML document separator inserted as part of `make-release-artifacts.sh` to the top of each manifest rather than the bottom.

Having the separator at the end of the manifest results in the generated release YAML files containing an empty document at the end. Whilst this is valid YAML, it is untidy, and certain YAML tools (`yq`) have been seen to handle empty document elements badly.
@dsbibby dsbibby requested a review from a team as a code owner April 7, 2022 14:58
@google-cla
Copy link

google-cla bot commented Apr 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@xtineskim
Copy link
Contributor

thanks for the PR @dsbibby! This LGTM,
@NimJay is it ok if merge?

@NimJay
Copy link
Collaborator

NimJay commented Apr 12, 2022

Sounds good to me!
I have not manually tested these changes.
But if anything breaks, we will find when we create the next release (via these steps) — which I will likely do in the coming weeks. Any breakage won't impact any users (or other stakeholders) of this repo.
Merging.

Thanks, @ckim328!
And thanks, @dsbibby!

@NimJay NimJay merged commit e95f6ba into GoogleCloudPlatform:main Apr 12, 2022
@NimJay NimJay self-requested a review April 12, 2022 19:26
@dsbibby dsbibby deleted the patch-1 branch June 28, 2022 07:48
D-Mwanth pushed a commit to D-Mwanth/microservices-demo that referenced this pull request Mar 6, 2024
…Platform#799)

Move the YAML document separator inserted as part of `make-release-artifacts.sh` to the top of each manifest rather than the bottom.

Having the separator at the end of the manifest results in the generated release YAML files containing an empty document at the end. Whilst this is valid YAML, it is untidy, and certain YAML tools (`yq`) have been seen to handle empty document elements badly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants