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

ci: use gh in install.sh for artifacts download #516

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Jan 31, 2023

This PR aims to avoid hitting the API rate limiting by using gh which should already be using GITHUB_TOKEN on Github's CI.

@pmalek pmalek added the area/ci label Jan 31, 2023
@pmalek pmalek self-assigned this Jan 31, 2023
@pmalek pmalek force-pushed the use-gh-in-install.sh branch 2 times, most recently from 5960014 to 926d806 Compare January 31, 2023 15:36
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2023

Codecov Report

Base: 53.78% // Head: 53.88% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (88a37b0) compared to base (f41d719).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #516      +/-   ##
==========================================
+ Coverage   53.78%   53.88%   +0.10%     
==========================================
  Files          50       50              
  Lines        3901     3901              
==========================================
+ Hits         2098     2102       +4     
+ Misses       1543     1539       -4     
  Partials      260      260              
Flag Coverage Δ
integration-test 58.86% <ø> (+0.11%) ⬆️
unit-test 3.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/clusters/utils.go 51.86% <0.00%> (+1.65%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pmalek pmalek marked this pull request as ready for review January 31, 2023 15:41
@pmalek pmalek requested a review from a team as a code owner January 31, 2023 15:41
@pmalek pmalek requested a review from shaneutt January 31, 2023 15:41
shaneutt
shaneutt previously approved these changes Feb 2, 2023
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Let's just make sure we add helpful output if a user uses this script but doesn't have gh installed to go install it, and then test it on our supported platforms, then LGTM 👍

@pmalek
Copy link
Member Author

pmalek commented Feb 2, 2023

Let's just make sure we add helpful output if a user uses this script but doesn't have gh installed to go install it, and then test it on our supported platforms, then LGTM 👍

Thanks for the suggestion: I ended up testing this on Mac arm64 and Linux aarch64 (as reported by uname) alpine image. This needed some changes.

PTAL.

@pmalek pmalek requested a review from shaneutt February 2, 2023 15:48
shaneutt
shaneutt previously approved these changes Feb 2, 2023
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Looks OK, just as long as we test before merging.

I would also ask if we haven't already, we should create some automated tests for this installer and to exercise the CLI to ensure we're not breaking the CLI and its installer on updates. If we could at minimum create a follow-up issue for that, and manually test now that would be good 👍

@pmalek
Copy link
Member Author

pmalek commented Feb 2, 2023

I've added a very simple installer test: example run: https://github.com/Kong/kubernetes-testing-framework/actions/runs/4076445875/jobs/7024196104#step:3:10

Plus: as noted above I've tested it manually which should do for now. The old behaviour of downloading via curl stays, so there shouldn't ™️ be any surprises there.

PTAL.

@pmalek pmalek requested a review from shaneutt February 2, 2023 16:32
@pmalek
Copy link
Member Author

pmalek commented Feb 2, 2023

Force merging because @shaneutt is not a member of team required to approve PRs in this repo 🤷

edit: shaneutt retroactively added to CODEOWNERS as per 8f02401

@pmalek pmalek merged commit 25f8331 into main Feb 2, 2023
@pmalek pmalek deleted the use-gh-in-install.sh branch February 2, 2023 16:56
run: ./docs/install.sh

- name: run ktf
run: ~/.local/bin/ktf --help
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of --help here? 🤔

if the purpose is to validate that the ktf binary is present, please (at least) comment about the intent behind using --help in an automated workflow


if [ "$ARCH" != "amd64" ]; then
if [[ "$ARCH" != "amd64" && "$ARCH" != "arm64" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: inconsistent quotes around arch names "arm64" vs aarch64 in lines 37 and 33 respectively

docs/install.sh Show resolved Hide resolved
docs/install.sh Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants