github-cli: simplify logic for detecting available gh releases #318

mislav · 2022-12-01T16:05:55Z

This change relies on git ls-remote --tags --sort=version:refname to sort git versions in a semver-compatible mode and alleviate the need to scan for version numbers manually. The new approach selects at most two latest releases that match VERSION and installs the first .deb that was successfully downloaded from a GitHub CLI release.

This also removes support for installing a specific version number with apt-get install gh=VERSION, since the GitHub CLI package repository does not retain older gh versions and only allows installing the latest one.

Finally, this removes version-selecting tests that only tested an implementation that was entirely contained within tests.

Bonuses:

Only install curl, apt-transport-https, dirmngr, gnupg2 when needed (the default installDirectlyFromGitHubRelease mode does not need them).
Ensure apt-get update runs at most once.
Remove the side-effect of /var/lib/apt/lists being wiped out when the github-cli feature is activated.
Add ability to export bash functions for testing: TEST_EXPORT=find_version_from_git_tags ./src/github-cli/install.sh
Errors are printed to stderr.

Followup to #310

/cc @samruddhikhandale: I thought I'd take a stab at reducing the size of the implementation so that the github-cli feature is easier to maintain over time. I hope this can be useful! 🙇

This change relies on `git ls-remote --tags --sort=version:refname` to sort git versions in a semver-compatible mode and alleviate the need to scan for version numbers manually. The new approach selects at most two latest releases that match VERSION and installs the first `.deb` that was successfully downloaded from a GitHub CLI release. This also removes support for installing a specific version number with `apt-get install gh=VERSION`, since the GitHub CLI package repository does not retain older gh versions and only allows installing the latest one. Finally, this removes version-selecting tests that only tested an implementation that was entirely contained within tests. Bonuses: - Only install curl, apt-transport-https, dirmngr, gnupg2 when needed. - Ensure `apt-get update` runs at most once. - Remove the side-effect of `/var/lib/apt/lists` being wiped out when the github-cli feature is activated. - Add ability to export bash functions for testing: `TEST_EXPORT=find_version_from_git_tags ./src/github-cli/install.sh` - Errors are printed to stderr.

mislav · 2022-12-01T16:09:27Z

src/github-cli/install.sh

+none )
+    # opt out of installing gh
+    exit 0
+    ;;
+latest | lts | stable | current )


Is my interpretation of "version": "none" and "version": "current" correct? I wasn't sure how those values are supposed to be handled since they aren't documented

The current proposals property mentions latest and none. But if the gihutb-cli supports other keywords, then it's nice to add. Thanks!

Feel free to update the proposals here as well - here

From existing code, it looked like github-cli feature supported many different keywords, but I only want to support the old ones for backwards compatibility and not actively suggest their use. I do not even understand what none is about. The usages I'd like to document are:

"version": "latest" - default

"version": "2.20.1" - lock to a specific version

"version": "2.20" - install the latest 2.20.x

"version": "2" - install the latest 2.x

Any suggestions how to best document this?

I see, for version option we are accepting semver values so I think it should be self explanatory of how versions are computed. However, it's great to document and make the usage clearer. Feel free to add a NOTES.md file in https://github.com/devcontainers/features/tree/main/src/github-cli. The action would automatically add the contents to the README file.

(example - docker-in-docker)

samruddhikhandale

This is awesome! Thank you for providing a cleaner and sleek approach for detecting releases. Much appreciated 👏

I have left some comments/questions. Would appreciate if you could help add tests to validate this change. Also, looks like there is one failing test 😄

samruddhikhandale · 2022-12-01T18:28:23Z

src/github-cli/install.sh

@@ -9,6 +9,7 @@

 CLI_VERSION=${VERSION:-"latest"}
 INSTALL_DIRECTLY_FROM_GITHUB_RELEASE=${INSTALLDIRECTLYFROMGITHUBRELEASE:-"true"}
+TEST_EXPORT=${TEST_EXPORT:-}


Add ability to export bash functions for testing: TEST_EXPORT=find_version_from_git_tags ./src/github-cli/install.sh

Could you help me explain the scenarios in which this is helpful? I read this part ☝️ , but usually tests for Features are run with devcontainers features test cmd. Curious to understand your thought behind it's usage 😄

I used this "export" mechanism in my development to manually test a single bash function from the install.sh script without running the whole install.sh script (which wouldn't even work on macOS). I noticed that you copied over bash functions for testing into the test.sh script, and with this mechanism you could possibly export functions for testing in CI rather that copying over the functions. However, I haven't made use of that in the current PR, and I'm not really attached to the TEST_EXPORT mechanism either. We could also just drop it from the final implementation.

Makes sense. I agree that if we aren't planning to make use of it, it's great to drop the changes. thanks!

samruddhikhandale · 2022-12-01T18:32:42Z

src/github-cli/install.sh

@@ -17,11 +18,8 @@ keyserver hkp://keyserver.pgp.com"

 set -e

-# Clean up


This PR #210 intentionally added the cleaning up of old packages at the start and end of the script (for all Features) which makes sense to me. Hence, each Feature cleans up the cache and starts afresh. Do you have any other thoughts where this would not be helpful ?

Thanks for linking to the more general PR. I did not realize this pattern spans multiple different features.

I found it odd that simply activating the github-cli feature would have side-effects on the apt-get mechanism, even if apt-get update wasn't invoked at all. Also, /var/lib/apt/lists is an implementation detail of apt-get and I don't think it's healthy for scripts like ours to rely on it in the long run. Finally, I find that cleaning the /var/lib/apt/lists cache after the script is done is unnecessary as caches exist for a reason. So my suggestion would be to just do apt-get update once, then apt-get install a few times to get what we need, and to leave everything else to the system.

samruddhikhandale · 2022-12-01T18:33:23Z

src/github-cli/install.sh

-        echo "Running apt-get update..."
-        apt-get update -y
-    fi
+ran_apt_get_update="false"


If we decide to re-add the Cleanup, we could remove this change as well 😄

samruddhikhandale · 2022-12-01T18:36:52Z

src/github-cli/install.sh

-                declare -g ${variable_name}="${major}.${minor}.${breakfix}"
-            fi
+    local prefix="${1:-}"
+    git ls-remote --tags --sort=-version:refname https://github.com/cli/cli.git | while read -r _ ref; do


Woah! This looks so much cleaner and nicer 👏

Wondering, if you could add few test scenarios to validate that these changes work as expected?

We have some guidance on how write tests here - https://github.com/devcontainers/cli/blob/main/docs/features/test.md#scenarios

Also, this repo has quite a few samples (eg - dotnet)

Thanks for the pointers! I'll try to do that

samruddhikhandale · 2022-12-01T18:37:49Z

src/github-cli/install.sh

+    # the exact version is specified explicitly
+    versions=( "$CLI_VERSION" )
+    ;;
+* )


Can we also add another case for *.* ? that's something users might use as well.

Good point, but the case for *.* is already handled by the general fallback case *, which will also match *.*.

samruddhikhandale · 2022-12-01T18:40:09Z

src/github-cli/install.sh

+none )
+    # opt out of installing gh
+    exit 0
+    ;;
+latest | lts | stable | current )


The current proposals property mentions latest and none. But if the gihutb-cli supports other keywords, then it's nice to add. Thanks!

Feel free to update the proposals here as well - here

samruddhikhandale · 2022-12-01T18:43:57Z

src/github-cli/install.sh

    . /etc/os-release
    receive_gpg_keys GITHUB_CLI_ARCHIVE_GPG_KEY /usr/share/keyrings/githubcli-archive-keyring.gpg
    echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" > /etc/apt/sources.list.d/github-cli.list
    apt-get update
-    apt-get -y install "gh${version_suffix}"
+    apt-get -y install gh


Good to know. Thanks!

Can we also add a description to installDirectlyFromGitHubRelease property to prompt that?

Something like 👇

If false, it installs the latest version of GitHub CLI

mislav · 2022-12-02T14:30:59Z

Looking at the CI failure: it looks like the git version on Bionic is too old to support the --sort=version:refname option. I'll update the PR to achieve semantic version sorting differently than relying on this git feature.

samruddhikhandale · 2022-12-14T18:06:24Z

Hi @mislav 👋

This PR would be super useful and will improve the current Feature. Let me know if I could somehow help to push it through. thanks!

mislav · 2022-12-14T18:09:49Z

@samruddhikhandale Thanks; I intend to finish this PR soon by addressing review comments.

Address case where git is old (Bionic)
Restore apt-get cleanup step (for consistency with other features in this repo)
Add more scenarios to tests
Remove TEST_EXPORT hack (since we will be using scenarios for integration testing)

mislav · 2023-05-08T14:45:59Z

Thanks for all your guidance @samruddhikhandale, but I'm going to close this because it's no longer needed. In GitHub CLI we've just changed our deployment process so that release assets are published at the same time as the git tag itself. Therefore, there isn't a period of time anymore where a git tag exists but the assets do not exist, so there is no need for a fallback mechanism to select the previous git tag.

samruddhikhandale · 2023-05-08T15:33:11Z

Nice, that's awesome. Thank you for the update!

mislav requested a review from a team as a code owner December 1, 2022 16:05

mislav commented Dec 1, 2022

View reviewed changes

samruddhikhandale reviewed Dec 1, 2022

View reviewed changes

mislav closed this May 8, 2023

samruddhikhandale mentioned this pull request May 22, 2023

docker-in-docker: buildx release not found #556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

github-cli: simplify logic for detecting available gh releases #318

github-cli: simplify logic for detecting available gh releases #318

mislav commented Dec 1, 2022 •

edited

Loading

mislav Dec 1, 2022

samruddhikhandale Dec 1, 2022

mislav Dec 2, 2022

samruddhikhandale Dec 2, 2022

samruddhikhandale left a comment •

edited

Loading

samruddhikhandale Dec 1, 2022

mislav Dec 2, 2022

samruddhikhandale Dec 2, 2022

samruddhikhandale Dec 1, 2022

mislav Dec 2, 2022

samruddhikhandale Dec 1, 2022

samruddhikhandale Dec 1, 2022

mislav Dec 2, 2022

samruddhikhandale Dec 1, 2022

mislav Dec 2, 2022 •

edited

Loading

samruddhikhandale Dec 1, 2022

samruddhikhandale Dec 1, 2022

mislav commented Dec 2, 2022

samruddhikhandale commented Dec 14, 2022

mislav commented Dec 14, 2022

mislav commented May 8, 2023

samruddhikhandale commented May 8, 2023

		@@ -17,11 +18,8 @@ keyserver hkp://keyserver.pgp.com"

		set -e

		# Clean up

github-cli: simplify logic for detecting available gh releases #318

github-cli: simplify logic for detecting available gh releases #318

Conversation

mislav commented Dec 1, 2022 • 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

samruddhikhandale left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav commented Dec 2, 2022

samruddhikhandale commented Dec 14, 2022

mislav commented Dec 14, 2022

mislav commented May 8, 2023

samruddhikhandale commented May 8, 2023

mislav commented Dec 1, 2022 •

edited

Loading

samruddhikhandale left a comment •

edited

Loading

mislav Dec 2, 2022 •

edited

Loading