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.

Sign up for GitHub

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

Jump to bottom

don't check in mounter binary #47497

Merged
merged 1 commit into from
Nov 9, 2017
Merged

don't check in mounter binary #47497

merged 1 commit into from
Nov 9, 2017

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Jun 14, 2017 •
edited 

GCI mounter is moved from the manifests tarball to the server tarball.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 14, 2017
@mikedanese
Copy link
Member Author

/approve no-issue

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 14, 2017
@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 14, 2017
@mikedanese mikedanese assigned ixdy and unassigned gmarek and vishh Jun 14, 2017
@mikedanese
Copy link
Member Author

So we have a checked in binary and removing it breaks the build...

@ixdy
Copy link
Member

ixdy commented Jun 14, 2017

what is using it?

@ixdy
Copy link
Member

ixdy commented Jun 14, 2017

it looks like mounter was a shell script until #41944.

assuming the mounter binary is the output of mounter.go, we should probably integrate this into the build process, or move it out-of-tree...

cc @jingxu97 @vishh @msau42

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 18, 2017
@mikedanese mikedanese force-pushed the binary branch 2 times, most recently from b0f786d to 78a286c Compare July 18, 2017 18:29
@kubernetes kubernetes deleted a comment from k8s-ci-robot Jul 18, 2017
@spxtr
Copy link
Contributor

spxtr commented Jul 18, 2017

That's nice.

@ixdy
Copy link
Member

ixdy commented Jul 18, 2017

incidentally, #48969 will start building the mounter binary every time for bazel builds.

@mikedanese
Copy link
Member Author

/retest

@mikedanese mikedanese requested review from rohitagarwal003 and removed request for rohitagarwal003 July 19, 2017 04:37
@spxtr
Copy link
Contributor

spxtr commented Jul 21, 2017

Please rebase.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2017
@mikedanese
Copy link
Member Author

@ixdy rebased.

@ixdy
Copy link
Member

ixdy commented Oct 26, 2017

LGTM, though you can probably squash the commits, since they don't really make sense as separate pieces anymore.

@ixdy
Copy link
Member

ixdy commented Oct 26, 2017

Test failure looks legitimate:

I1026 18:46:41.613] unable to create test archive: failed to setup test package "/tmp/node-e2e-archive031296679": Could not find GCI/COS mounter script at "/go/src/k8s.io/kubernetes/cluster/gce/gci/mounter/mounter"! If this script has been (re)moved, please update the e2e node remote test runner accordingly! Err: stat /go/src/k8s.io/kubernetes/cluster/gce/gci/mounter/mounter: no such file or directory.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 26, 2017
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2017
@mikedanese
Copy link
Member Author

/retest

@mikedanese mikedanese assigned BenTheElder and unassigned spxtr Nov 8, 2017
ixdy
ixdy reviewed Nov 9, 2017
View reviewed changes
test/e2e_node/builder/build.go
@@ -34,6 +34,7 @@ var buildTargets = []string{
"cmd/kubelet",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes me sad that these are not just bazel dependencies.

ixdy
ixdy reviewed Nov 9, 2017
View reviewed changes
test/e2e_node/remote/node_e2e.go
@@ -110,51 +104,11 @@ func tarAddFile(tar, source, dest string) error {
return nil
}

// Includes the GCI/COS mounter artifacts in the deployed tarball
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much less code. yes!

@ixdy
Copy link
Member

ixdy commented Nov 9, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017
@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@ixdy
Copy link
Member

ixdy commented Nov 9, 2017

Do you think it warrants a release note that the gci mounter binary was moved from the manifests tarball to the server tarball?

@k8s-ci-robot k8s-ci-robot 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 Nov 9, 2017
@mikedanese
Copy link
Member Author

Added a release note.

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy, mikedanese

Associated issue requirement bypassed by: mikedanese

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 54773, 52523, 47497, 55356, 49429). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f7dc396 into kubernetes:master Nov 9, 2017
@mikedanese mikedanese deleted the binary branch November 9, 2017 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ixdy ixdy ixdy left review comments

Assignees

@BenTheElder BenTheElder

@ixdy ixdy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@mikedanese @ixdy @spxtr @msau42 @jingxu97 @k8s-github-robot @BenTheElder @gmarek @vishh @k8s-ci-robot