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

feat: CI action / label that makes it easy to cherry-pick just a commit against cosmos SDK main #12478

Merged
merged 40 commits into from
Jul 27, 2022

Conversation

xBalbinus
Copy link
Contributor

@xBalbinus xBalbinus commented Jul 7, 2022

Description

Closes: #268

Adds a Github action that makes it easy to cherry pick commits from a fork PR to main under fork-cherry-pick.yml. The necessity for this PR arises from Mergify / GitHub marketplace currently not having tooling for this kind of action.

How it works:

  • Cherry picks the commit of any fork of cosmos-sdk upon a merged PR to main on fork.
  • Puts commit into a pr-patch branch; makes PR from pr-patch to cosmos:main

Author Checklist

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • [-] included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@xBalbinus xBalbinus requested a review from a team as a code owner July 7, 2022 14:57
@alexanderbez alexanderbez changed the title [ci] A CI action / label that makes it easy to cherry-pick just a commit against cosmos SDK main feat: CI action / label that makes it easy to cherry-pick just a commit against cosmos SDK main Jul 7, 2022
@tac0turtle
Copy link
Member

Cherry picks the commit of any fork of cosmos-sdk upon a merged PR to main on fork.

do we have to go to the repo to tag prs? what is the label we need to request teams to create?

@xBalbinus
Copy link
Contributor Author

xBalbinus commented Jul 9, 2022 via email

@xBalbinus
Copy link
Contributor Author

xBalbinus commented Jul 9, 2022 via email

@alexanderbez
Copy link
Contributor

So we have to essentially merge this PR to see if it really works, correct?

@tac0turtle
Copy link
Member

https://github.com/peepmaster/cosmos-sdk/runs/7390327769?check_suite_focus=true

This is the latest CI run that proves that this 'works'. I'm getting a 403 error from the API for "Forbidden" using the Pull Request API - I think we may have to merge to test this but would love to know what others think of the cause / fixes of the 403.

ah this is because external people don't get keys injected into ci for security purposes by GitHub actions

@xBalbinus
Copy link
Contributor Author

xBalbinus commented Jul 26, 2022 via email

@xBalbinus
Copy link
Contributor Author

Actually sorry, I think I do understand now and that's exactly to our discussion earlier that this needs to be merged in to test.

@alexanderbez
Copy link
Contributor

Ok, @marbar3778 are you OK with merging first and seeing if this works? If it doesn't, we either patch it or revert it.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

ack

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 27, 2022
@tac0turtle tac0turtle self-assigned this Jul 27, 2022
@mergify mergify bot merged commit 9ffd57a into cosmos:main Jul 27, 2022
@julienrbrt
Copy link
Member

Is it supposed to run at each PR? I don't think it works https://github.com/cosmos/cosmos-sdk/actions/workflows/fork-cherry-pick.yml

@tac0turtle
Copy link
Member

cc @xBalbinus ^^

@xBalbinus
Copy link
Contributor Author

xBalbinus commented Jul 27, 2022 via email

@xBalbinus
Copy link
Contributor Author

After a bit of investigation @marbar3778 @julienrbrt , it seems that there's an issue with the origin of the PR?

The way the action is supposed to function is:

  • User merges a PR into the main branch of a FORK of cosmos/sdk
  • The same PR gets cherry picked into a PR to the main branch of cosmos/sdk

I think what I need to do here is actually also add another trigger limiter that says this should only trigger from FORK PRs, not main?

@julienrbrt
Copy link
Member

Alright but why this action is on the Cosmos SDK repo then? If I understand properly it's to the fork that wants to submit PR of their changes that should have this action instead of the Cosmos SDK itself.

@xBalbinus
Copy link
Contributor Author

To my understanding the original reason was that the action would then be generalizable across all forks of cosmos SDK...

@xBalbinus
Copy link
Contributor Author

I've temporarily disabled the workflow as I debug, will re-enable when set.

@tac0turtle
Copy link
Member

I think it should be disabled at all times on the sdk, then when a fork appears like osmosis, they can reenable it

@julienrbrt
Copy link
Member

julienrbrt commented Jul 27, 2022

I understand, makes sense.

One more question, what happens if there is two PRs from a same fork? It seems that the branch name is hardcoded to pr-patch, should not we have the commit ID in the branch name for ensuring uniqueness?

@xBalbinus
Copy link
Contributor Author

@marbar3778 Agreed! Thank you for the very good idea.

And @julienrbrt you are totally right on that as well. I will make the branch name pr-patch- to make each unique to avoid bumping into issues like that via. a PR.

Thank you all for your help!

@julienrbrt
Copy link
Member

julienrbrt commented Jul 27, 2022

While you are at it can you add a name to the workflow :) So it will display better on GitHub.

@xBalbinus
Copy link
Contributor Author

Yessir! You got it.

mergify bot pushed a commit that referenced this pull request Aug 15, 2022
## Description

A sequel to #12478

Adds a Github action that makes it easy to cherry pick commits from a fork PR to main under fork-cherry-pick.yml. The necessity for this PR arises from Mergify / GitHub marketplace currently not having tooling for this kind of action.

Changelog:
- Added templating for the commit-branch that the action creates on the fork.
- Added a name to the Github action.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@xBalbinus
Copy link
Contributor Author

xBalbinus commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. Type: CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make a CI action / label, that makes it easy to cherry-pick just a commit against cosmos SDK main
4 participants