-
Notifications
You must be signed in to change notification settings - Fork 18
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
Extract git history from go-kit/kit/log #2
Conversation
* Initial import, minus logrus and zap * GitHub Actions for CI * gofmt * Satisfy staticcheck * It was a nice idea * Satisfy staticcheck
The commit history for this branch is unusual because it has multiple root commits. Of course git can handle it fine, but it makes for a strange PR due to the lack of a common merge base with the main branch. How much do we care about that? If we want a more typical looking commit history I'm sure we could make that happen with a little history surgery and a force push to the main branch. That is usually an unsavory step, but it seems OK to me given the warning label @peterbourgon put in the initial README and the intentions of extracting the module here. My vote would be for cleaning up the history, and I'm willing to do the work. I would stage it in a repo under my account for review before force pushing it here, but I'll wait to see what others think before proceeding. |
Please take the reins. |
I have created a private repo to stage my work and sent invites to @peterbourgon and @sagikazarmark so you can each review the commit history there. The procedure I applied was to start with 5e1beb4 and rebase the extract branch onto it to get a linear history of the work @peterbourgon has already done here. Then I interactively rebased the last few commits to squash some redundant history and update the commit messages to reflect the changes actually made in the updated commits. The updated commit history eliminates the annoying
If I get sign off on that history I can bring it over to this repo with a force push to the main branch and we can proceed from there. |
Since this is a new repository and not an existing one, I think you can skip merging unrelated branches. In my case I had to merge the tree into an existing repo, so it made sense there, probably doesn't in this case. The history looks good to me. Can I ask what's the reason behind dropping those packages? I have a few guesses of course (eg. don't want to maintain them/add them as a dependency), but I think I would move the last commit to a separate PR to better document it. If someone looks at the repo, they won't find any reasoning why those packages were dropped. |
Basically they pull in additional dependencies which the vast majority of consumers will not use, and nothing they do really require them to be maintained here. |
That's what I though, but I'd still add a note about that somewhere. People coming from go-kit will only notice that it's missing and won't know if that's intentional, lost in the migration, etc. A PR is a good way to document something like this. |
That is a good suggestion. I can push that commit as a separate branch and we can make it into a PR. |
Fine by me. |
OK. I will push the updated history here soon. I will create a new branch to preserve the existing commits until we confirm everything is good. |
Check that, the existing |
I can't easily predict — will the force push blow away the v0.0.1-alpha tag? |
No, the tag is a separate thing and will also preserve the commit it tags. It would have to be deleted separately. Also, it looks like I don't have permissions to push to the main branch on this repo or access to change that. So I'll need some help. |
@ChrisHines You should have them now, or perhaps an invite pending to gain them. |
OK. New main branch pushed and PR #3 created to trim out packages we are dropping. |
Closing now that #3 is merged. |
To be clear — we are deliberately not deleting the |
I followed this guide (thanks @sagikazarmark!) and I think got everything working...