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

Add ODH release workflow #17

Merged
merged 1 commit into from
Oct 25, 2024
Merged

Conversation

oksanabaza
Copy link

@oksanabaza oksanabaza commented Oct 9, 2024

What this PR does / why we need it:

Adding a release workflow in the ODH fork of Training Operator

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #

https://issues.redhat.com/browse/RHOAIENG-13631

This workflow has been tested locally using my personal GH token here

Checklist:

  • Docs included if any changes are user facing

Copy link

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of small comments.

.github/workflows/odh-release.yaml Outdated Show resolved Hide resolved
.github/workflows/odh-release.yaml Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 18, 2024

Pull Request Test Coverage Report for Build 11439915191

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 42.785%

Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/mpi/mpijob_controller.go 10 80.48%
Totals Coverage Status
Change from base Build 11236886062: -0.1%
Covered Lines: 3745
Relevant Lines: 8753

💛 - Coveralls

Copy link

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

/lgtm

@ChughShilpa
Copy link

@oksanabaza
Did you try the run this workflow manually? Asking to make sure the github token GITHUB_TOKEN: ${{ secrets.CODEFLARE_MACHINE_ACCOUNT_TOKEN }} works effectively

@oksanabaza
Copy link
Author

Hey @ChughShilpa , this is a good point, I haven't as I need a token to test it

@ChughShilpa
Copy link

Hey @ChughShilpa , this is a good point, I haven't as I need a token to test it

You can test it by adding a token in your fork and then try running the workflow

@oksanabaza
Copy link
Author

oksanabaza commented Oct 23, 2024

@ChughShilpa I tested the workflow with my personal GitHub token in my fork, and it worked fine. However, when I tried to run the workflow in the fork using the CODEFLARE_MACHINE_ACCOUNT_TOKEN from the original repository, I got a 404 error. It seems that the CODEFLARE_MACHINE_ACCOUNT_TOKEN may not have the right permissions to be used in forks

@ChughShilpa
Copy link

@ChughShilpa I tested the workflow with my personal GitHub token in my fork, and it worked fine. However, when I tried to run the workflow in the fork using the CODEFLARE_MACHINE_ACCOUNT_TOKEN from the original repository, I got a 404 error. It seems that the CODEFLARE_MACHINE_ACCOUNT_TOKEN may not have the right permissions to be used in forks

Yes right, CODEFLARE_MACHINE_ACCOUNT_TOKEN doesn't work in fork repos

@ChughShilpa
Copy link

@ChughShilpa I tested the workflow with my personal GitHub token in my fork, and it worked fine. However, when I tried to run the workflow in the fork using the CODEFLARE_MACHINE_ACCOUNT_TOKEN from the original repository, I got a 404 error. It seems that the CODEFLARE_MACHINE_ACCOUNT_TOKEN may not have the right permissions to be used in forks

Just FYI, it is good to add the workflow link which you ran manually in the description of the PR stating how you tested this PR

Copy link

@ChughShilpa ChughShilpa left a comment

Choose a reason for hiding this comment

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

/lgtm

@oksanabaza
Copy link
Author

thanks @ChughShilpa, I will update the description

Copy link

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@Fiona-Waters Fiona-Waters merged commit cc52281 into opendatahub-io:dev Oct 25, 2024
5 of 16 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