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.

Sign up for GitHub

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

Jump to bottom

Bot to pull changes from upstream #558

Closed
wants to merge 6 commits into from
Closed

Bot to pull changes from upstream #558

wants to merge 6 commits into from

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Oct 8, 2021 •
edited
Loading

Description

This PR introduced a workflow that pulls changes from upstream and the v0.34.x release branch twice a week.

This PR is blocked by having a github repository secret that is named PULL_UPSTREAM that has permissions for repo read and write, along with access to workflows.

we can see a working version on this fork

Closes: #528

Note: sometimes it fails when there are too many conflicts, but we have to handle conflicts by hand anyway.

@evan-forbes
Copy link
Member Author

I didn't expect to run into this many hurdles just setting up a simple github PR bot... @Bidon15 have you encountered any of this before? am I missing something simple?

@Bidon15
Copy link
Member

Bidon15 commented Oct 12, 2021

Hey @evan-forbes, I haven't encountered to such behaviours, yet. It's really weird though.
I've deep dived into some of mentioned above. We can actually discuss or setup a session to work things out for the create-pr-from-upstream-repos-for-private-forks as it looks so dead simple.
My gut feeling is that it needs some tweaks to make it work for our case as it's just a bash script with simple ifs at the end of the day

@evan-forbes
Copy link
Member Author

I managed to get the workflow in this PR to work on my fork of tendermint, but it won't work on my fork of celestia-core

I finally figured it out last night, it was just a permissions issue. The bot needs permission to edit workflows, along with reading and writing to the repo. Otherwise, if there are commits that change the .github/workflows repo, then it won't be able to create the PR.

@evan-forbes evan-forbes self-assigned this Oct 12, 2021
@evan-forbes evan-forbes marked this pull request as ready for review October 12, 2021 22:19
adlerjohn
adlerjohn reviewed Oct 13, 2021
View reviewed changes
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Is there any ways to make it so that modified workflows don't run until manually allowed?

@evan-forbes
Copy link
Member Author

Is there any ways to make it so that modified workflows don't run until manually allowed?

idk, that's a good question. I'll look into it. I guess that's where we'd have to review them before we merge the PR.

@adlerjohn
Copy link
Member

adlerjohn commented Oct 14, 2021 •
edited
Loading

I guess that's where we'd have to review them before we merge the PR.

The issue is that if workflows run during the PR, then upstream can maliciously modify a workflow to e.g. exfiltrate CI keys. So either we make sure there are none of those (maybe not possible?), or stop CI from running on PRs the bot makes.

@evan-forbes
Copy link
Member Author

evan-forbes commented Oct 15, 2021 •
edited
Loading

That sounds terrible. Perhaps we should just have a bot open an issue to remind us, and then we can run a script and handle all conflicts by hand (something we'll have to do anyway)? If it's only once a week, I don't think that would be too much work.

This was referenced Oct 15, 2021
Closed
Merged
Merged
@adlerjohn
Copy link
Member

I'm not sure which way's the best to handle this. Any opinions @liamsi?

@evan-forbes
Copy link
Member Author

after thinking about it a bit more, I think we should just auto open an issue to make sure we don't forget, and then manually merge. Most of the time, there will be at least one conflict, which will force us to pull the branch and fix the them by hand anyway.

still curious to hear what others think. In the mean time, I'll switch this to a draft.

@evan-forbes evan-forbes marked this pull request as draft October 16, 2021 21:56
@evan-forbes evan-forbes mentioned this pull request Oct 16, 2021
Closed
@Bidon15
Copy link
Member

Bidon15 commented Oct 18, 2021 •
edited
Loading

I guess that's where we'd have to review them before we merge the PR.

The issue is that if workflows run during the PR, then upstream can maliciously modify a workflow to e.g. exfiltrate CI keys. So either we make sure there are none of those (maybe not possible?), or stop CI from running on PRs the bot makes.

If our point of concern is CI keys(secrets) being exposed by such malicious behaviour, then I propose we have a look 👁️ on Hashicorp's Vault

TL;DR for Vault - dynamic secrets and we can audit and revoke if we see malicious behaviour or the key has been leaked in some way or another 🗝️ 🧐

They have even released a GH Action for Vault. 😮

wdyt?

@evan-forbes evan-forbes mentioned this pull request Nov 9, 2021
Closed
13 tasks
@evan-forbes
Copy link
Member Author

I'll close this for now, unless we decide otherwise. With each update, there was almost always a conflict that had to be handled manually, so there's no real point in having a bot imho

@evan-forbes evan-forbes closed this Dec 3, 2021
@evan-forbes evan-forbes deleted the evan/pull-upstream-bot branch September 2, 2022 04:50
evan-forbes pushed a commit that referenced this pull request Jun 9, 2023
@mergify @lasarojc
crypto/merkle: Add error handling (backport #558) (#710)
1641d39 
* crypto/merkle: Add error handling (#558)

* porting pr

* Disallow nil root hashes

* linting

* making computeRootHash private

* Update release notes.

* Update .changelog/unreleased/breaking-changes/558-tm10011.md

Co-authored-by: Thane Thomson <connect@thanethomson.com>

* Wrapping the private function into a public one with the old signature.

* update comment

* remove dependency on amino

* Update .changelog/unreleased/breaking-changes/558-tm10011.md

Co-authored-by: Thane Thomson <connect@thanethomson.com>

---------

Co-authored-by: Thane Thomson <connect@thanethomson.com>
(cherry picked from commit d067b740ec543718ba36865375acdfbb7d1116cd)

# Conflicts:
#	crypto/merkle/proof.go
#	crypto/merkle/proof_test.go

* fix conflicts

---------

Co-authored-by: Lasaro <lasaro@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adlerjohn adlerjohn adlerjohn left review comments

@Bidon15 Bidon15 Awaiting requested review from Bidon15

@liamsi liamsi Awaiting requested review from liamsi

@Wondertan Wondertan Awaiting requested review from Wondertan

Assignees

@evan-forbes evan-forbes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make it easy to pull changes from upstream and cherry-pick celestia-core onto tendermint
3 participants
@evan-forbes @Bidon15 @adlerjohn