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

Change directory to the given path #563

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

kfcampbell
Copy link
Collaborator

@kfcampbell kfcampbell commented Nov 15, 2023

Provide a way to commit to other repositories, not just the one the Action is run from. Do that by allowing a Node process.chdir call to a given directory.

index.js Outdated Show resolved Hide resolved
@kfcampbell kfcampbell marked this pull request as draft November 15, 2023 20:59
index.js Outdated
const { hasChanges } = await getLocalChanges(inputs.path);
if (inputs.path) {
core.debug(`Changing directory to ${inputs.path}`);
process.chdir(inputs.path);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a red flag? Are there other options I might use? I picked process.chdir because runShellCommand invokes a new terminal session each time it's run, and that defaults to the directory the code is being run from.

@kfcampbell
Copy link
Collaborator Author

kfcampbell commented Nov 15, 2023

@gr2m before I edit the tests, are you okay with the direction here? It's worked to create PRs in another repo: octokit/go-sdk#6

Our use case here is we have a separate repository for our code generation and for where the generated code lives. Is there something I'm missing about how this could be improved (outside of removing the dist/index.js file?

@kfcampbell
Copy link
Collaborator Author

@gr2m I believe these test errors are due to my fork's GITHUB_TOKEN not having access to your repository. See these partially successful tests on my own fork.

Do you have thoughts about this PR?

@kfcampbell
Copy link
Collaborator Author

I've done some more research on the test cases here, and I'm pretty confused.

First, on this PR, the tests deterministically fail:

Each test fails due to a 403 error attempting to create PRs on this repo. I believe pretty strongly these errors are due to the fact that this PR is from a fork, and the auto-generated GITHUB_TOKEN doesn't have access to Gregor's repo. See an example log here.

On my branch cd-path on my fork with the Actions run outside the PR, the tests deterministically fail across 5 runs like so:

  • Create new PR and auto merge always succeed
  • The other tests fail with either one of two errors: a 422 PR has already been created, and a git error "branch already exists" when attempting to create a new branch.
  • The Add reviewers test fails each time, which is expected since @gr2m is not a collaborator on my repo.
    See an example log here.

On the master branch on my fork, running the Actions using the workflow_dispatch trigger, the tests are deterministic across 5 runs:

  • The first five tests succeed each time
  • The sixth test (update PR title and body) fails each time with the error "Pull request title is \"Test pull request\" but expected \"Updated test pull request\""
  • The seventh test fails each time with the error Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the kfcampbell/create-or-update-pull-request-action repository.
    • This is expected since @gr2m is hard-coded into the tests

See an example log here.

On the master branch on the original repo here, tests are not deterministic across 5 runs but fail each time:

  • First run: Create multiple commits fails with a 422 PR already exists error. Add reviewers fails too, but that fails across all runs and will henceforth be ignored.
  • Second run: Update existing PR fails with a git error: cannot lock ref 'refs/heads/create-or-update-pull-request-action': is at 806579b373f9d06645ba8d8aaac3eb934f1962da but expected f2e5778b69818e5d281727155f5b7c6efda09b36.
  • Third run: all tests succeed except for add reviewers, which is always broken.
  • Fourth run: update existing PR fails with a similar ref locking error as run 2.
  • Fifth run: create or update PR for hidden file fails with a similar ref locking error as run 2.

What next?

I'm guessing that the expected failures and normal behavior is somewhere between the results of the master branches. I've proposed #595 so we can see the changes from within the repository compared to a fork. It appears the results are the same as when the Action is run from the branch on my fork.

My next step will be to reason about the code further and attempt to make code changes to get the test failures closer to the test failures we see on master here.

@kfcampbell
Copy link
Collaborator Author

This PR (or #595, they're the same content) is now passing all expected tests and is ready for review!

@kfcampbell kfcampbell marked this pull request as ready for review December 6, 2023 19:52
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