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

git: accept explicit commit hash for git context #1765

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

wwade
Copy link
Contributor

@wwade wwade commented Oct 16, 2021

When checking out code from non-github repositories, the typical
assumptions may not be valid, e.g. that the only interesting
non-branch commits have ref names starting with refs/pull. A specific
example is fetching an un-merged commit from a gerrit repository by
commit hash.

This change just looks at the second part of the git context path and
checks if it's a SHA commit hash, and if so, will fetch and check out
this commit after cloning the repository.

Sample context argument:

git://github.repo/project#e1772f228e06d15facdf175e5385e265b57068c0

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

  • Support explicit commit hash when using a Git Repository context.

@google-cla google-cla bot added the cla: yes CLA signed by all commit authors label Oct 16, 2021
@tejal29
Copy link
Contributor

tejal29 commented Oct 19, 2021

@wwade can you please rebase. Sorry for the delay in reviewing.

I fixed the tests on master/main branch #1766

@wwade wwade force-pushed the master branch 2 times, most recently from 5314064 to 1061be9 Compare October 19, 2021 21:27
@wwade wwade closed this Oct 19, 2021
@wwade wwade reopened this Oct 19, 2021
@wwade
Copy link
Contributor Author

wwade commented Oct 19, 2021

@wwade can you please rebase. Sorry for the delay in reviewing.

No worries, rebase is done now.

I fixed the tests on master/main branch #1766

Excellent news, and I see that you've switched to Actions, now.

Thanks for having a look.

@wwade
Copy link
Contributor Author

wwade commented Oct 20, 2021

I'm just going to fix the "Unit tests" failure also affecting "build-executor".

wwade added 2 commits October 19, 2021 18:59
When checking out code from non-github repositories, the typical
assumptions may not be valid, e.g. that the only interesting
non-branch commits have ref names starting with refs/pull. A specific
example is fetching an un-merged commit from a gerrit repository by
commit hash.

This change just looks at the second part of the git context path and
checks if it's a SHA commit hash, and if so, will fetch and check out
this commit after cloning the repository.

Sample context argument:

    https://github.repo/project#e1772f228e06d15facdf175e5385e265b57068c0
hack/linter.sh didn't properly install golangci-lint in hack/bin as I
already have another version of golangci-lint on my PATH, but then it
failed to execute because it was looking for it specifically in
hack/bin.

When the executable is not found, the exit code is 127 instead of 1,
and so test.sh ignored the error.

Two fixes:

1. `test.sh`:
  - Use `if (script) ...` instead of assigning / checking a result
    variable to determine if each validation script passed or failed.

2. `hack/linter.sh`:
  - Instead of checking for golangci-lint on the path, just
    specifically check for an executable file (`test -x`) in the
    expected location.
@wwade
Copy link
Contributor Author

wwade commented Oct 20, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants