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

Use PR head ref in output export #193

Merged
merged 5 commits into from
Dec 15, 2022
Merged

Use PR head ref in output export #193

merged 5 commits into from
Dec 15, 2022

Conversation

ofalvai
Copy link
Contributor

@ofalvai ofalvai commented Dec 15, 2022

Checklist

  • I've read and followed the Contribution Guidelines
  • step.yml and README.md is updated with the changes (if needed)

Version

Requires a PATCH version update

Context

With #186, the step checks out the PR merge ref and uses that state to export outputs, such as the commit hash and commit message. This might be confusing for users (previous discussions: #89, #165), it makes more sense to use the PR head ref for output exports.

Changes

  • Fetch the PR head ref in addition to the PR merge ref. We do it with the same fetch params, so this should be a shallow fetch in most cases.
  • Use the local head ref for exporting outputs
  • Remove an unused parameter from the checkout method

Investigation details

Decisions

@ofalvai ofalvai requested a review from hisaac December 15, 2022 09:17
`git "fetch" "--jobs=10" "--no-tags" "--no-recurse-submodules" "origin" "refs/pr_test:refs/remotes/pr_test"`,
`git "checkout" "refs/remotes/pr_test"`,
},
wantErrType: ParameterValidationError{},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case is a little weird, I don't think it's valid that a build is triggered with a merge ref but without a head ref, so I changed the test to expect an error.

Copy link
Contributor

@hisaac hisaac left a comment

Choose a reason for hiding this comment

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

LGTM. Good thinking.

@ofalvai ofalvai merged commit 9a440cc into next Dec 15, 2022
@ofalvai ofalvai deleted the ACI-222-output-head-ref branch December 15, 2022 15:51
@ofalvai ofalvai restored the ACI-222-output-head-ref branch December 15, 2022 16:08
@ofalvai ofalvai deleted the ACI-222-output-head-ref branch December 15, 2022 16:19
ofalvai added a commit that referenced this pull request Jan 3, 2023
* Use PR head ref in output export

* Remove unused parameter

* Fix constructor

* Define head ref in E2E tests
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.

2 participants