-
Notifications
You must be signed in to change notification settings - Fork 173
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
Sign old commits prior to enforcing commit signature #53
Comments
@jeff-mccoy is this still something we need to do? If so is it something you have to do or can one of us do it with a little guidance? |
Assigning @jeff-mccoy since I believe he is specifically the person that needs to do this. |
I think you do too 😂, but thanks I'll do mine. |
Oh? I'm not aware of me needing to do anything. AFAIK I've been doing signed commits since I started in August since we turned on the requirement right around that time. I think this is to go back and sign old commits prior to August, which would have just been you and Josh |
there are 4 commits at the beginning from Josh, initially we had two separate repos working similar problems, pushed everything to his repo so we could work together, but they ended up moving to another project instead. There's actually no code left from that initial work he pushed so not sure if we need to have him sign it or just squash it. I have most of the missing sigs early on, but you have some too: https://github.com/defenseunicorns/zarf/commits?after=acd5dc445c5131a7afd39ec7acbde8d308d27caa+34&author=RothAndrew |
Gotcha. I'll go take a look at those. https://superuser.com/a/1123928 is still the right way to do it? We'll have to turn off the branch protection on master to force push to it. |
I think so but let's wait post-merge of the big pr because I don't want to know what a force push will do to the rebasing on that branch. |
We could, like, not do it at all 😜 . Force pushing to change history always gives me the willies. |
Nice...but no we need proper attribution of code in the repo. |
Okay now that we had to do a force-push tonight to get #237 merged without breaking it's signatures, I think this is the right time to also fix this issues to avoid another force-push in the future. I’ve experimented with different ways to do this and think that using git rebase --signoff is the most logic choice. Basically every method causes some sort of rewrite, but this preserves the author, message, contents and timestamp for the commits, it's also a built-in feature of git. Some more notes on this can be found at https://stackoverflow.com/a/1962112/467373. I used the command You can see at the master-signature-test that this actually annotated every commit all the way back to the first one so now all have a verified signature. I did see that some merge-ceptions that say partially verified, but I think we'd have to go and rewrite those old branches to fix that issue as the secondary PRs themselves have unverified commits somehow. |
I should also mention even though this will rewrite history and all hashes, moving in-progress branches isn't super hard, just need to use reset soft + stash to move the affected files. The hardest part would be if you had a lot of commits ahead and you wanted to preserve them with messages, you'd have to do the same process but one reset/stash per commit. |
I'm not worried about in-progress branches with this approach. I think I need a little more info to understand what you mean by rewriting the old branches to fix the Either way, I think this is the best approach and something we should get out of the way sooner than later. |
That's a speculation on partially verified. What I was trying to say is that partially verified issue exists already and this branch doesn't immediately fix that. It has to do with how GitHub is evaluating PRs associated with a commit. This is not a git thing, this branch fixes the git signature issue properly. But GitHub has some magic it does for PRs tagged in a commit, if you click on one you'll see that's what's breaking. |
LGTM |
Okay I think if @RothAndrew concurs we'll proceed. |
LGTM |
This commit is the completion of a history rewrite to sign-off all unsigned commits in the repo from its inception. Please see the comments on #53 for more details on how to move your WIP if you have a branch you are working with commits ahead of master. Note that because this is a rewrite pulling master will return an error the first time unless you run `git pull --force`. Make sure you stash or commit your changes if you have anything on master locally that isn't merged already.
Confirmed with @RothAndrew @YrrepNoj and @mike-winberry, proceeding with the force-push |
LGTM too! |
Couple notes on updating master locally that I had to do (alternative could be just reclone or delete local master):
|
Can we call this backlog item done? |
Yeah I guess GitHub didn't know how to auto-close the tagged issue on history rewrite 😂 |
This commit is the completion of a history rewrite to sign-off all unsigned commits in the repo from its inception. Please see the comments on #53 for more details on how to move your WIP if you have a branch you are working with commits ahead of master. Note that because this is a rewrite pulling master will return an error the first time unless you run `git pull --force`. Make sure you stash or commit your changes if you have anything on master locally that isn't merged already.
We should scrub the commit history for old commits and verify their authenticity cryptographically to ensure a clean/validated git history. One possible way to do it: https://superuser.com/a/1123928
The text was updated successfully, but these errors were encountered: