-
Notifications
You must be signed in to change notification settings - Fork 35
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 api_hub_tests_ref to CI on PRs #559
Conversation
In order to run E2E API tests with tackle2-hub API tests on matching branches, adding name of PR branch as a variable passed to CI workflow. Depends on konveyor/ci#23 Related to konveyor/ci#22 and konveyor/go-konveyor-tests#61 Signed-off-by: Marek Aufart <maufart@redhat.com>
.github/workflows/main.yml
Outdated
@@ -94,3 +94,4 @@ jobs: | |||
uses: konveyor/ci/.github/workflows/global-ci.yml@main | |||
with: | |||
component_name: tackle2-hub | |||
api_hub_tests_ref: ${{ github.head_ref }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want github.ref_name
here instead, I think head_ref
will reference just the PR branch while ref_name
will reference the merged PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This workflow should affect only PRs. Once the change is merged the branch name (main and e.g. release-0.3) should be driven by already existing API_TESTS_REF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the difference is head_ref
targets /refs/pull/<PR #>/head
whereas ref_name
targets /refs/pull/<PR #>/merge
. Generally we want to test with merge so that we get the results of the tests if that PR merged. Can help catch issues that are introduced on the main branch after a PR is introduced. Let me know if I'm missing something or talking past you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point, I misuderstood it before, thanks! I will update.
Signed-off-by: Marek Aufart <maufart@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In order to run E2E API tests with tackle2-hub API tests on matching branches, adding name of PR branch as a variable passed to CI workflow.
Depends on konveyor/ci#23
Related to konveyor/ci#22 and konveyor/go-konveyor-tests#61