Release separated archives for each platform (#697) #699

zigarn · 2021-03-13T12:38:27Z

Fixes #697

In install instructions, use --archive (and therefore --manifest-url) to not download again the tarball when launching krew install command.

k8s-ci-robot · 2021-03-13T12:38:34Z

Welcome @zigarn!

It looks like this is your first PR to kubernetes-sigs/krew 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-03-13T12:38:39Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

zigarn · 2021-03-13T13:48:46Z

/assign @corneliusweig

corneliusweig

Thanks for taking this on! This is a welcome improvement.

Please take a look at my comments.

corneliusweig · 2021-03-13T20:54:29Z

.github/workflows/ci.yml

@@ -80,7 +80,7 @@ jobs:
      uses: svenstaro/upload-release-action@v2
      with:
        repo_token: ${{ secrets.GITHUB_TOKEN }}
-        file: out/krew.*
+        file: out/krew*


Suggested change

file: out/krew*

file: out/krew-*

This would exclude the krew.exe[.sha256] and krew.yaml

corneliusweig · 2021-03-13T21:07:32Z

docs/RELEASING_KREW.md

    KREW_ROOT="$(mktemp -d --tmpdir krew-XXXXXXXXXX)" KREW_OS=darwin \
-        $krew install --manifest=out/krew.yaml --archive=out/krew.tar.gz && \
+        $krew install --manifest=out/krew.yaml --archive=out/krew-darwin_amd64.tar.gz && \
    KREW_ROOT="$(mktemp -d --tmpdir krew-XXXXXXXXXX)" KREW_OS=linux \
-        $krew install --manifest=out/krew.yaml --archive=out/krew.tar.gz && \
+        $krew install --manifest=out/krew.yaml --archive=out/krew-linux_amd64.tar.gz && \
    KREW_ROOT="$(mktemp -d --tmpdir krew-XXXXXXXXXX)" KREW_OS=windows \
-        $krew install --manifest=out/krew.yaml --archive=out/krew.tar.gz
+        $krew install --manifest=out/krew.yaml --archive=out/krew-windows_amd64.tar.gz


Can you please do something like this instead:

for osarch in darwin_amd64 darwin_arm64 linux_amd64 linux_arm linux_arm64 windows_amd64; do KREW_ROOT="$(mktemp -d --tmpdir krew-XXXXXXXXXX)" KREW_OS="${osarch%_*}" \ $krew install --manifest=out/krew.yaml --archive="out/krew-${osarch}.tar.gz" done

I thought about it but didn't know if not everything was tested on purpose.

Oh! It was on purpose to not mix architectures. Will add KREW_ARCH.

site/content/docs/user-guide/setup/install.md

corneliusweig · 2021-03-13T21:13:49Z

site/content/docs/user-guide/setup/install.md

+      KREWNAME="krew-${OS}_${ARCH}" &&
+      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/${KREWNAME}.tar.gz" &&
+      tar zxvf "${KREWNAME}.tar.gz" &&
+      ./"${KREWNAME}" install --manifest-url https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.yaml --archive ./${KREWNAME}.tar.gz


Suggested change

./"${KREWNAME}" install --manifest-url https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.yaml --archive ./${KREWNAME}.tar.gz

./"${KREWNAME}" install krew

The --manifest and --archive options are only for testing. When a plugin is installed like this, it will be in a detached state where it is not updated when new versions are published on krew-index. So we do need to download twice here.

Arf, too bad.
Maybe would be possible to allow --archive without --manifest[-url] to use local archive?

That does not work either, I'm afraid. It should not be a big deal though. Installation is only done once, and you are already cutting down the download size.

Looks like it works, by removing the lines doing the check in cmd/krew/cmd/install.go and compiling.

diff --git i/cmd/krew/cmd/install.go w/cmd/krew/cmd/install.go index a574346..ac4b265 100644 --- i/cmd/krew/cmd/install.go +++ w/cmd/krew/cmd/install.go @@ -96,10 +96,6 @@ Remarks: return errors.New("must specify either specify either plugin names (via positional arguments or STDIN), or --manifest/--manifest-url; not both") } - if *archiveFileOverride != "" && *manifest == "" && *manifestURL == "" { - return errors.New("--archive can be specified only with --manifest or --manifest-url") - } - var install []pluginEntry for _, name := range pluginNames { indexName, pluginName := pathutil.CanonicalPluginName(name)

$ curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.tar.gz" $ ./out/bin/krew-linux_amd64 install krew --archive ./krew.tar.gz Adding "default" plugin index from https://github.com/kubernetes-sigs/krew-index.git. Updated the local copy of plugin index. Installing plugin: krew Installed plugin: krew \ | Use this plugin: | kubectl krew | Documentation: | https://krew.sigs.k8s.io/ | Caveats: | \ | | krew is now installed! To start using kubectl plugins, you need to add | | krew's installation directory to your PATH: | | | | * macOS/Linux: | | - Add the following to your ~/.bashrc or ~/.zshrc: | | export PATH="${KREW_ROOT:-$HOME/.krew}/bin:$PATH" | | - Restart your shell. | | | | * Windows: Add %USERPROFILE%\.krew\bin to your PATH environment variable | | | | To list krew commands and to get help, run: | | $ kubectl krew | | For a full list of available plugins, run: | | $ kubectl krew search | | | | You can find documentation at | | https://krew.sigs.k8s.io/docs/user-guide/quickstart/. | / / $ tail ~/.krew/receipts/krew.yaml matchLabels: arch: amd64 os: windows sha256: a26deea175f70264260d59a4e061778a892f8a8e301ac261660dd7d24c551c99 uri: https://github.com/kubernetes-sigs/krew/releases/download/v0.4.1/krew.tar.gz shortDescription: Package manager for kubectl plugins. version: v0.4.1 status: source: name: default

Source is not 'detached'.
And I can confirm tat there is no download of archive through the install.

I think we reserved those arguments specifically for local testing so they are used for installing "detached" plugins. I’m not sure if it's worth changing that just to prevent downloading a small tarball once more.

site/content/docs/user-guide/setup/install.md

corneliusweig · 2021-03-13T21:19:26Z

site/content/docs/user-guide/setup/install.md

-      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.tar.gz" &&
-      tar zxvf krew.tar.gz &&
-      KREW=./krew-"${OS}_${ARCH}" &&
-      "$KREW" install krew
+      KREWNAME="krew-${OS}_${ARCH}" &&
+      curl -fsSLO "https://github.com/kubernetes-sigs/krew/releases/latest/download/${KREWNAME}.tar.gz" &&
+      tar zxvf "${KREWNAME}.tar.gz" &&
+      ./"${KREWNAME}" install --manifest-url https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.yaml --archive ./${KREWNAME}.tar.gz


Changes in this file will affect the installation instructions seen by users for the current release. However, these instructions only become effective, once we craft a new release with the other changes within this PR. So the doc updates must go into a different PR that will be merged after the next release.
So please extract this into a separate PR.

Extracted in #700

Previous comments on it have been applied.

corneliusweig · 2021-03-13T21:27:49Z

hack/make-release-artifacts.sh

+  (
+    cd "${archive_dir}"
+    # consistent timestamps for files in archive dir to ensure consistent checksums
+    TZ=UTC touch -amt "0001010000" ./*


I imagine this could make trouble on Mac/BSD, because the man says

-a change only the access time
-m change only the modification time

So I'm worried they take the only too seriously. If you can check it works then great.

However, we can still make the change, because nowadays the release is only done by GH actions and they run on Linux where I have verified that the result is the same.

The doc start with:

Update the access and modification times of each FILE to the current time.

So I guess the only means that this option will change this one only compared to both.
And at the end, using both options is same as using none...

:D good catch

corneliusweig · 2021-03-13T21:39:44Z

hack/make-release-artifacts.sh

+  archive_dir="$(mktemp -d)"
+  cp "$f" "${archive_dir}"
+  cp -- "${SCRIPTDIR}/../LICENSE" "${archive_dir}"
+  archive="$(basename "$f" .exe).tar.gz"


Let's extract this into a variable, because it is reused a few times.

Suggested change

archive="$(basename "$f" .exe).tar.gz"

name=$(basename "$f" .exe)

archive="${name}.tar.gz"

corneliusweig · 2021-03-13T21:49:59Z

hack/make-release-artifacts.sh

-  checksum_cmd="sha256sum"
-fi
+  # prepare krew manifest sed
+  checksum_sed="${checksum_sed};s/KREW_$(basename "$f" .exe | sed 's/krew-\(.*\)/\U\1/')_CHECKSUM/${checksum}/"


Let's make the intent clearer by using tr here:

Suggested change

checksum_sed="${checksum_sed};s/KREW_$(basename "$f" .exe | sed 's/krew-$.*$/\U\1/')_CHECKSUM/${checksum}/"

checksum_sed="${checksum_sed};s/$(tr "[[:lower:]-]" "[[:upper:]_]" <<< ${name}

)_CHECKSUM/${checksum}/"

Ah ah, I did hesitate between both.

corneliusweig · 2021-03-13T21:54:51Z

hack/make-release-artifacts.sh

@@ -75,6 +76,6 @@ if [[ ! "${git_describe}" =~ v.* ]]; then
 fi
 krew_version="${TAG_NAME:-$git_describe}"
 cp ./hack/krew.yaml ./out/krew.yaml
-sed -i "s/KREW_TAR_CHECKSUM/${tar_checksum}/g" ./out/krew.yaml
+sed -i "${checksum_sed}" ./out/krew.yaml


Suggested change

sed -i "${checksum_sed}" ./out/krew.yaml

sed "${checksum_sed}" ./hack/krew.yaml > ./out/krew.yaml

And remove the cp ./hack/krew.yaml ./out/krew.yaml in the previous line

Did also integrated the next sed in it.

Co-authored-by: Cornelius Weig <corneliusweig@users.noreply.github.com>

corneliusweig

Works flawlessly, thanks! The only thing that I could not verify is that our release automation uploads the right files. It looks right though.

/lgtm
/approve
/hold
In case @ahmetb wants to take a look too.

corneliusweig · 2021-03-14T21:35:09Z

docs/RELEASING_KREW.md

-    KREW_ROOT="$(mktemp -d --tmpdir krew-XXXXXXXXXX)" KREW_OS=windows \
-        $krew install --manifest=out/krew.yaml --archive=out/krew.tar.gz
+    for osarch in darwin_amd64 darwin_arm64 linux_amd64 linux_arm linux_arm64 windows_amd64; do
+      KREW_ROOT="$(mktemp -d --tmpdir krew-XXXXXXXXXX)" KREW_OS="${osarch%_*}" KREW_ARCH="${osarch#*_}" \


:) I didn't even think of setting KREW_ARCH. Great that we do now!

k8s-ci-robot · 2021-03-14T21:55:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: corneliusweig, zigarn

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:

~~OWNERS~~ [corneliusweig]

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

fejta-bot · 2021-06-13T17:11:58Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

zigarn · 2021-06-13T17:13:15Z

/remove-lifecycle stale

corneliusweig · 2021-06-13T21:24:25Z

/hold cancel
Sorry, this slipped through. Feel free to be more impatient and ping me next time :)

zigarn · 2021-06-15T04:15:10Z

@corneliusweig, it has to wait for a release anyway, so no reason to be impatient.
BTW, any ETA for next release?

Linked #699

k8s-ci-robot requested review from ahmetb and corneliusweig March 13, 2021 12:38

k8s-ci-robot assigned corneliusweig Mar 13, 2021

corneliusweig reviewed Mar 13, 2021

View reviewed changes

Release separated archives for each platform (#697)

7a97601

Co-authored-by: Cornelius Weig <corneliusweig@users.noreply.github.com>

zigarn mentioned this pull request Mar 14, 2021

Adapt install doc for separated archives for each platform (#697) #700

Merged

zigarn requested a review from corneliusweig March 14, 2021 14:18

corneliusweig reviewed Mar 14, 2021

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 14, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 13, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 13, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2021

k8s-ci-robot merged commit 4c16711 into kubernetes-sigs:master Jun 13, 2021

zigarn deleted the issue-697 branch June 15, 2021 04:12

chriskim06 mentioned this pull request Oct 14, 2021

Unable to update to the lastest stable release on macOS 11.6 #723

Closed

k8s-ci-robot pushed a commit that referenced this pull request Oct 14, 2021

Adapt install doc for separated archives for each platform (#697) (#700)

cfab2bc

Linked #699

This was referenced Oct 14, 2021

tar download link does not work for v0.4.2 #724

Closed

Download links for v0.4.2 release does not work #726

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release separated archives for each platform (#697) #699

Release separated archives for each platform (#697) #699

zigarn commented Mar 13, 2021 •

edited

Loading

k8s-ci-robot commented Mar 13, 2021

k8s-ci-robot commented Mar 13, 2021

zigarn commented Mar 13, 2021

corneliusweig left a comment

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021

zigarn Mar 14, 2021

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021

corneliusweig Mar 14, 2021

zigarn Mar 15, 2021

ahmetb Mar 15, 2021

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021 •

edited

Loading

corneliusweig Mar 14, 2021

corneliusweig Mar 13, 2021

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021

corneliusweig Mar 13, 2021

zigarn Mar 14, 2021

corneliusweig left a comment

corneliusweig Mar 14, 2021

k8s-ci-robot commented Mar 14, 2021

fejta-bot commented Jun 13, 2021

zigarn commented Jun 13, 2021

corneliusweig commented Jun 13, 2021

zigarn commented Jun 15, 2021

	./"${KREWNAME}" install --manifest-url https://github.com/kubernetes-sigs/krew/releases/latest/download/krew.yaml --archive ./${KREWNAME}.tar.gz
	./"${KREWNAME}" install krew

	archive="$(basename "$f" .exe).tar.gz"
	name=$(basename "$f" .exe)
	archive="${name}.tar.gz"

	checksum_sed="${checksum_sed};s/KREW_$(basename "$f" .exe \| sed 's/krew-\(.*\)/\U\1/')_CHECKSUM/${checksum}/"
	checksum_sed="${checksum_sed};s/$(tr "[[:lower:]-]" "[[:upper:]_]" <<< ${name}
	)_CHECKSUM/${checksum}/"

	sed -i "${checksum_sed}" ./out/krew.yaml
	sed "${checksum_sed}" ./hack/krew.yaml > ./out/krew.yaml

Release separated archives for each platform (#697) #699

Release separated archives for each platform (#697) #699

Conversation

zigarn commented Mar 13, 2021 • edited Loading

k8s-ci-robot commented Mar 13, 2021

k8s-ci-robot commented Mar 13, 2021

zigarn commented Mar 13, 2021

corneliusweig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zigarn Mar 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corneliusweig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 14, 2021

fejta-bot commented Jun 13, 2021

zigarn commented Jun 13, 2021

corneliusweig commented Jun 13, 2021

zigarn commented Jun 15, 2021

zigarn commented Mar 13, 2021 •

edited

Loading

zigarn Mar 14, 2021 •

edited

Loading