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 workflow to Trigger build on PR add LGTM and Create Tag and Release with Changelog and push image to quay #366

Merged
merged 7 commits into from
Jun 20, 2024

Conversation

rpancham
Copy link

What this PR does / why we need it:
Automation PR builds/image for Kserve Repo

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

@openshift-ci openshift-ci bot requested review from Jooho and spolti May 31, 2024 14:32
@rpancham
Copy link
Author

@Jooho @israel-hdez @spolti I am unsure of the quay credential varibles as i have no access to check the secret variables i have reffered this workflow and have added kserve-controller-docker-publish.yml. Please let me know if they are wrong

if: ${{ needs.wait-checks.result == 'success' }}
run: |
gh pr comment ${{ needs.create-pr.outputs.pr_number }} --body "/lgtm"
gh pr edit ${{ needs.create-pr.outputs.pr_number }} --add-label lgtm
Copy link
Member

Choose a reason for hiding this comment

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

the label is added anyways if someone comments /lgtm

Copy link
Author

Choose a reason for hiding this comment

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

The label is indeed added if someone comments with /lgtm. However, our plan, based on prior discussions, was to automate this process to ensure consistency and efficiency.That said, it's not an issue to exclude it if necessary

echo "has_lgtm=${has_lgtm}" >> $GITHUB_OUTPUT
break
has_lgtm=true
echo "has_lgtm=${has_lgtm}" >> $GITHUB_OUTPUT
Copy link
Member

Choose a reason for hiding this comment

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

where else is this used?

Copy link
Author

Choose a reason for hiding this comment

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

"has_lgtm" variable isn't currently utilized in the workflow. During testing it on local, I experimented with incorporating it for the merge part of the code.I can remove it.

@@ -122,92 +121,92 @@ jobs:
if: ${{ needs.wait-checks.result == 'success' }}
run: |
gh pr comment ${{ needs.create-pr.outputs.pr_number }} --body "/lgtm"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do it automatically without reviewing?

Copy link
Member

Choose a reason for hiding this comment

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

is this create-pr coming from another workflow?

Copy link
Author

Choose a reason for hiding this comment

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

Not exactly. While we could exclude it from automation, the initial plan was to incorporate it automatically.

for i in {1..60}; do
with:
timeout_minutes: 120
max_attempts: 60
Copy link
Member

Choose a reason for hiding this comment

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

I guess GHA actions can be triggered based on events, this will be safer than a retry loop, wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

Yes we dont need wait-lgtm i had added this bit of code while testing on the Local will remove and push the code

@rpancham
Copy link
Author

rpancham commented Jun 7, 2024

/retest

@rpancham
Copy link
Author

rpancham commented Jun 7, 2024

/retest-required

@israel-hdez israel-hdez self-requested a review June 13, 2024 19:49
with:
registry: quay.io
username: ${{ secrets.QUAY_USERNAME }}
password: ${{ secrets.Q_PASSWORD }}

Choose a reason for hiding this comment

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

Should it be QUAY_PASSWORD?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah i have changed it even i am not sure of the variable name as i have no access to review the settings in the repo so i have reffered this fileLINK

Choose a reason for hiding this comment

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

In the configs, I see it is QUAY_USER and QUAY_PASSWORD.
So, you may want to change it to QUAY_USER, rather than QUAY_USERNAME.

I do not have access to the values, so I'm not sure if it can be renamed... Although, I should have the credentials stored somewhere... So, we may merge as it is and I can later setup the new variable name.

Copy link

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

@rpancham Let me know if you will rename QUAY_USERNAME or if we merge as is.

Copy link

openshift-ci bot commented Jun 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, rpancham

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [israel-hdez,rpancham]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@israel-hdez
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 20, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 9a16be4 into opendatahub-io:master Jun 20, 2024
19 checks passed
rpancham pushed a commit to rpancham/kserve that referenced this pull request Jun 24, 2024
Add workflow to Trigger build on PR add LGTM and Create Tag and Release with Changelog and push image to quay
rpancham pushed a commit to rpancham/kserve that referenced this pull request Jun 25, 2024
Add workflow to Trigger build on PR add LGTM and Create Tag and Release with Changelog and push image to quay
rpancham pushed a commit to rpancham/kserve that referenced this pull request Jun 25, 2024
Add workflow to Trigger build on PR add LGTM and Create Tag and Release with Changelog and push image to quay
rpancham pushed a commit to rpancham/kserve that referenced this pull request Jun 25, 2024
Add workflow to Trigger build on PR add LGTM and Create Tag and Release with Changelog and push image to quay
@rpancham rpancham deleted the pr-build branch July 10, 2024 10:27
@rpancham rpancham restored the pr-build branch July 10, 2024 10:27
rpancham pushed a commit to rpancham/kserve that referenced this pull request Jul 10, 2024
Add workflow to Trigger build on PR add LGTM and Create Tag and Release with Changelog and push image to quay
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants