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

[monorepo] Fix flytectl install script #5405

Merged
merged 4 commits into from
May 23, 2024
Merged

Conversation

eapolinario
Copy link
Contributor

Why are the changes needed?

The script used to install flytectl needs to be updated to reflect the move to the monorepo.

What changes were proposed in this pull request?

The install.sh script used to be generated by godownloader, but that tool has been deprecated for at least 2 years. I couldn't get it to work with relatively recent versions of go (I went as far as 1.18). Another problem is that the tool is not aware of the pattern we've been using to release flytectl from the monorepo (specifically the fact that releases have git tags containing the flytectl/ prefix).

Taking all that into consideration, there are two changes worth reviewing in this PR:

  1. We now require jq to be able to parse the valid versions of flytectl
  2. We use the official github api to list all releases, since the old endpoint used didn't return the flytectl releases (due to the fact that the tags associated with those releases have the flytectl/ prefix, e.g. https://github.com/flyteorg/flyte/releases/tag/flytectl%2Fv0.8.21).

Once this is merged we'll be able to modify the scarf entry that points to the install script to point to https://raw.githubusercontent.com/flyteorg/flyte/master/flytectl/install.sh.

How was this patch tested?

I tested locally in both linux and mac. I made sure to exercise the code paths for both when a version is specified and not specified.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Copy link

codecov bot commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.10%. Comparing base (2143948) to head (163b65b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5405   +/-   ##
=======================================
  Coverage   61.09%   61.10%           
=======================================
  Files         793      793           
  Lines       51156    51156           
=======================================
+ Hits        31256    31257    +1     
+ Misses      17028    17027    -1     
  Partials     2872     2872           
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.90% <ø> (ø)
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 68.31% <ø> (ø)
unittests-flyteidl 79.30% <ø> (ø)
unittests-flyteplugins 61.94% <ø> (ø)
unittests-flytepropeller 57.32% <ø> (ø)
unittests-flytestdlib 65.75% <ø> (+0.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wild-endeavor
wild-endeavor previously approved these changes May 22, 2024
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
@eapolinario eapolinario enabled auto-merge (squash) May 23, 2024 01:39
@eapolinario eapolinario merged commit 0583f77 into master May 23, 2024
52 of 53 checks passed
@eapolinario eapolinario deleted the fix-flytectl-install-script branch May 23, 2024 18:22
pmahindrakar-oss pushed a commit that referenced this pull request May 31, 2024
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>
eapolinario added a commit that referenced this pull request May 31, 2024
* Revert "Revert "Ensure token is refreshed on Unauthenticated (#5388)" (#5404)"

This reverts commit 7d2f0d0.

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* Using same mutex for condition variable

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* Lock the locker in the wait to adher to cond.Wait() semantics

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* comments

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* using noop locker as waitlist add is atomic operation

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* Replace Azure AD OIDC URL with correct one (#4075)

Signed-off-by: Erwin de Haan <erwin.de.haan@calcasa.nl>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* Update the example Dockerfile to run on k8s (#5412)

Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* docs(kubeflow): Fix kubeflow webhook error (#5410)

Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* update flytekit version to 1.12.1b2 in monodocs requirements (#5411)

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* Add supported task types to agent service config and rename (#5402)

Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* update lock file (#5416)

Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* [monorepo] Fix flytectl install script (#5405)

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* bring in changes for flytecl keyring from PR flytectl/pull/488

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* typo fix

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

---------

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>
Signed-off-by: Erwin de Haan <erwin.de.haan@calcasa.nl>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
Co-authored-by: Erwin de Haan <1627021+EraYaN@users.noreply.github.com>
Co-authored-by: Jason Parraga <Sovietaced@gmail.com>
Co-authored-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Co-authored-by: Samhita Alla <aallasamhita@gmail.com>
Co-authored-by: Eduardo Apolinario <653394+eapolinario@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants