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

Move from drone to Github Actions #133

Merged
merged 1 commit into from
May 2, 2024

Conversation

mitulshah-suse
Copy link
Contributor

@mitulshah-suse mitulshah-suse commented Apr 23, 2024

Move from drone to GHA.

  • removed the existing drone.yml
  • added new github workflow

ref: SURE-7991

Copy link
Member

@jiaqiluo jiaqiluo left a comment

Choose a reason for hiding this comment

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

The PR looks good to me overall, and I have one request for changes:
could you add the permissions to each job? AFAIK, all jobs need the permission contents: read to prevent privilege escalation, and the job that reads the credential needs the permission id-token: write (see example here).

@jiaqiluo jiaqiluo requested a review from a team April 29, 2024 18:26
@mitulshah-suse mitulshah-suse force-pushed the move-to-gha branch 3 times, most recently from 43c92c9 to 0a32c58 Compare April 30, 2024 07:22
@mitulshah-suse
Copy link
Contributor Author

mitulshah-suse commented Apr 30, 2024

@jiaqiluo Thanks for pointing that out. Have updated permissions for the jobs.
Also realised that GHA runs jobs in parallel by default instead of sequentially like drone. So added needs to the jobs to create dependency and run them sequentially. Had to merge a couple of jobs as well, since there is no gaurantee of everything executing on the same runner as well, so changes in 1 job would not necessarily reflect in the next.
Added a timeout for each of the jobs as well, since the default is 360 mins which is too long.

Also, do you know if we need to use the containerized runners specified by EIO?
ex: runs-on: org-${{ github.repository_owner_id }}-{{ arch }}-k8s or we can continue using the github provided ones like ubuntu-latest

@mitulshah-suse mitulshah-suse requested a review from jiaqiluo April 30, 2024 07:32
@mitulshah-suse mitulshah-suse force-pushed the move-to-gha branch 2 times, most recently from 7ba8ec8 to 8cb7c6a Compare April 30, 2024 09:53
@jiaqiluo
Copy link
Member

Also, do you know if we need to use the containerized runners specified by EIO?
ex: runs-on: org-${{ github.repository_owner_id }}-{{ arch }}-k8s or we can continue using the github provided ones like ubuntu-latest

Hi @mitulshah-suse, you continue using the GitHub provided ones.

@jiaqiluo jiaqiluo requested a review from a team April 30, 2024 18:07
needs: validate
runs-on: ubuntu-latest
timeout-minutes: 90
if: github.event_name == 'pull_request'

Choose a reason for hiding this comment

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

I'm generally a fan of creating multiple workflow files for different actions. For example, one for PRs, another for merges and one for tagging. IMO that pattern is easier to read and maintain, gets rid of skipped checks in the github UI when raising PRs, and allows us to avoid the use of if statements depending on what action is invoking the workflow. Adding conditionals like this can make it harder to determine what should and shouldn't execute given a particular on statement.

I don't work with this repo often, so I won't hold up this PR based off this, but it's something that should be considered.

@snasovich snasovich merged commit d6fe6bb into rancher:master May 2, 2024
4 checks passed
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.

4 participants