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

chore: Allow auto-pr to include multiple changes #1365

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nschonni
Copy link
Member

@nschonni nschonni commented Oct 9, 2020

Opened this as a draft since I noticed the auto-pr is creating a single commit and force pushing while the upstream PR is open.
I haven't tested this, so I'm opening as a draft for discussion. The idea would be to manually create the commits as described https://github.com/peter-evans/create-pull-request#controlling-commits and then just the let the action update/push the branch

/cc @ttshivers

@ttshivers
Copy link
Member

I like this. What is the || true for?

@nschonni
Copy link
Member Author

nschonni commented Oct 9, 2020

In the case of nothing to commit, git commit -am will exit non-zero and fail the job.

$ git commit -am "foo"
On branch allow-multiple-auto-pr-changes
Your branch is up to date with 'origin/allow-multiple-auto-pr-changes'.

nothing to commit, working tree clean

@nschonni
Copy link
Member Author

nschonni commented Oct 9, 2020

Other minor thoughts on this one:

  • Possibly keep title generic
  • Append comments/body when updating existing PR

Those might be more hassle than benefit to add

@ttshivers
Copy link
Member

What if instead of ... || true, it was something like git diff-index --quiet HEAD || git commit ..., so that real errors do cause a fail.
Source: https://stackoverflow.com/a/8123841

@nschonni nschonni force-pushed the allow-multiple-auto-pr-changes branch from fe89bd2 to 101d07f Compare October 9, 2020 00:38
@ttshivers
Copy link
Member

ttshivers commented Oct 9, 2020

Other minor thoughts on this one:

  • Possibly keep title generic
  • Append comments/body when updating existing PR

Those might be more hassle than benefit to add

For updating PR body, let me see if there is a good way to find the associated PR.

Edit: One way is just using the output of the PR action and editing it afterwards.

@nschonni
Copy link
Member Author

nschonni commented Oct 9, 2020

Thanks! that is a better idea

@nschonni
Copy link
Member Author

nschonni commented Oct 9, 2020

@ttshivers yeah, that's why I was thinking it might be more trouble that it's worth, since github already has the relationship from the comment on the closed PR in this repo. Alternately, that part could just be removed from the PR body in official images, but doesn't really matter

@ttshivers
Copy link
Member

ttshivers commented Oct 9, 2020

Ah, there is an issue with this atm. The node ref is on the fork, not on the main repo. The PR action uses the repo checked out as the place it makes its PR. If the forked repo was checked out, it wouldn't make the PR to the official one.

@ttshivers
Copy link
Member

Looking at the docs (https://github.com/peter-evans/create-pull-request), I think a strategy like this might work:

  • Check out official repo still
  • git remote add fork https://github.com/nodejs-github-bot/official-images
  • git fetch fork
  • git checkout -b node --track fork/node

Then in the action, specify base: master to make it open the PR against that branch on the official-images repo

@nschonni nschonni force-pushed the allow-multiple-auto-pr-changes branch from 101d07f to dabcb93 Compare October 9, 2020 01:09
@ttshivers
Copy link
Member

ttshivers commented Oct 12, 2020

I think you might need to add base: master in the action config, otherwise I think it will try and open a PR to a branch named node since that is what is checked out. https://github.com/peter-evans/create-pull-request#action-inputs

base Sets the pull request base branch. Defaults to the branch checked out in the workflow.

@ttshivers
Copy link
Member

Also, running some tests for this action (with it pointing to different repos). I notice that it may be required to do:

  git config user.email "you@example.com"
  git config user.name "Your Name"

instead of just doing it inline in the commit.

See: https://github.com/ttshivers/docker-node/runs/1245462778?check_suite_focus=true#step:6:1

@ttshivers
Copy link
Member

Using the node branch that is tracked by the fork is not working with this PR action. I've opened an issue on the PR to see if there can be an easy workaround made: peter-evans/create-pull-request#609

However, we might just need to write our own logic to create / update the PR instead.

Base automatically changed from master to main March 15, 2021 16:23
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