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 kubernetes-src.tgz has kubernetes/ prefix #78667

Merged

Conversation

@dims
Copy link
Member

commented Jun 3, 2019

There's a difference between the kubernetes-src.tgz build from
make bazel-release and make quick-release. The quick-release does
not have a kubernetes/ prefix and hence essentiall tarbomb(s) the
directory when someone tries to untar it.

Change-Id: I8e87639d85dd01aec534b58f1d5740bd48ac922f

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes kubernetes/website#13728

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE
Ensure kubernetes-src.tgz has kubernetes/ prefix
There's a difference between the kubernetes-src.tgz build from
`make bazel-release` and `make quick-release`. The quick-release does
not have a `kubernetes/` prefix and hence essentiall tarbomb(s) the
directory when someone tries to untar it.

Change-Id: I8e87639d85dd01aec534b58f1d5740bd48ac922f
@dims

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

/sig release
/assign @tpepper @cblecker

@dims

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

/priority important-soon
/milestone v1.15

@@ -115,7 +115,7 @@ function kube::release::package_src_tarball() {
\) -prune \
\))
)
"${TAR}" czf "${src_tarball}" -C "${KUBE_ROOT}" "${source_files[@]}"
"${TAR}" czf "${src_tarball}" --transform 's|^\.|kubernetes|' -C "${KUBE_ROOT}" "${source_files[@]}"

This comment has been minimized.

@BenTheElder
Copy link
Member

left a comment

/lgtm

@@ -115,7 +115,7 @@ function kube::release::package_src_tarball() {
\) -prune \
\))
)
"${TAR}" czf "${src_tarball}" -C "${KUBE_ROOT}" "${source_files[@]}"
"${TAR}" czf "${src_tarball}" --transform 's|^\.|kubernetes|' -C "${KUBE_ROOT}" "${source_files[@]}"

This comment has been minimized.

Copy link
@cblecker

cblecker Jun 3, 2019

Member

Would this be safer? I'm assuming this is what this does.

Suggested change
"${TAR}" czf "${src_tarball}" --transform 's|^\.|kubernetes|' -C "${KUBE_ROOT}" "${source_files[@]}"
"${TAR}" czf "${src_tarball}" --transform 's|^\./|kubernetes/|' -C "${KUBE_ROOT}" "${source_files[@]}"

This comment has been minimized.

Copy link
@dims

dims Jun 4, 2019

Author Member

@cblecker the original code works fine. from my trial and error the second one adds an extra '/' i believe. we should be ok with what we had originally

@dims

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

/test pull-kubernetes-integration

@cblecker

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

The other question I have is, do we need this for 1.15? How long has this been the case? Are we going to break any other workflows that use the src tarball because we're changing the published format?

@dims

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@cblecker i searched for references and did not find anywhere we are using this tgz. but yes i could have missed something. it's something easy to revert (or fix), so i think we should try for 1.15

@cblecker

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@dims I don't know if it'll be that quick to fix if it's part of the published release artifacts.. we'd have to re-publish them if we found some dependency that broke.

@dims

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@cblecker we still have beta.2 and rc.1 left to shake it out. but yes, we can defer this to 1.16, but the situation won't be different there either ..

@cblecker

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dims

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit d1e828f into kubernetes:master Jun 4, 2019

21 checks passed

cla/linuxfoundation dims authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Thanks @dims :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.