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

Ensure the GCI metadata files do not have newline at the end #38727

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

Amey-D
Copy link
Contributor

@Amey-D Amey-D commented Dec 13, 2016

Fixes #36708

@dchen1107 @Random-Liu can one of you help with the review?

cc @kubernetes/goog-image FYI

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Dec 13, 2016
@dchen1107 dchen1107 assigned dchen1107 and unassigned mikedanese Dec 13, 2016
@dchen1107
Copy link
Member

LGTM

@Random-Liu Random-Liu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Dec 13, 2016
@Random-Liu
Copy link
Member

Random-Liu commented Dec 13, 2016

@Amey-D Thanks for the fix.

I use vim to open a file, and type update_disabled. Then use this file as metadata. It won't work.

However, echo -n update_disabled does work.

I need to figure out what is added by vim. :/

@Random-Liu
Copy link
Member

Random-Liu commented Dec 13, 2016

OK. Vim adds EOL at the end of each line automatically http://stackoverflow.com/questions/16222530/why-do-i-need-vim-in-binary-mode-for-noeol-to-work/16224292#16224292.

If I generate the file with echo -n update_disabled, it will work.
If I open the same file with vim, and save it, it won't work anymore.

Anyhow, at least it's clear that this PR will work! :)

However, I still think we should fix GCI if possible because this is so fragile and confusing. :/

@yujuhong
Copy link
Contributor

yujuhong commented Dec 13, 2016

The title of the PR should be "trailing newline" instead of whitespace :-)
Please also add comments to prevent regression since the logic is a bit fragile.

+1 to make the consumer of this metadata more robust. It'd also help if we surface the information/configuration. Could we add something to our e2e test framework to collect such information or add a test for GCI?

@dchen1107 dchen1107 changed the title Ensure the GCI metadata files do not have whitespace at the end Ensure the GCI metadata files do not have newline at the end Dec 13, 2016
@dchen1107
Copy link
Member

Agreed with @Random-Liu and @yujuhong the proper and more robust fix should at GCI side, not here. But I am ok to take this for short-term fix for running GCI on GCE.

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2016
@dchen1107 dchen1107 added this to the v1.5 milestone Dec 13, 2016
@dchen1107
Copy link
Member

Offline discussion with @Amey-D:

  • We are going to fix this in GCI side for long term. @Amey-D is going to file a separate internal bug to track this. The fix in GCI is required for GCI is running outside GKE anyway.
  • The current fix as the short term solution should be cherrypicked into both 1.4 and 1.5 release.

cc/ @kubernetes/goog-node @kubernetes/goog-image @saad-ali for 1.5 @jessfraz for 1.4

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@jessfraz
Copy link
Contributor

thanks!

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f826e11 into kubernetes:master Dec 14, 2016
@jessfraz
Copy link
Contributor

cherry-picked in #38781

@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 14, 2016
k8s-github-robot pushed a commit that referenced this pull request Dec 15, 2016
…27-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #38727

Cherry pick of #38727 on release-1.4.

#38727: Ensure the GCI metadata files do not have whitespace at the
@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jan 20, 2017
@saad-ali
Copy link
Member

Cherry-picked to 1.5 in #40184

k8s-github-robot pushed a commit that referenced this pull request Jan 20, 2017
…27-upstream-release-1.5

Automatic merge from submit-queue

Automated cherry pick of #38727 upstream release 1.5

Automated cherry pick PR #38727 ("Ensure the GCI metadata files do not have whitespace at the end") to `release-1.5` branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants