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

Revert "fix: improve docker set up" #3923

Merged
merged 26 commits into from
Jan 11, 2025
Merged

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Jan 11, 2025

Reverts #3921

@aeneasr aeneasr requested a review from a team as a code owner January 11, 2025 12:31
@aeneasr aeneasr merged commit a9930c0 into master Jan 11, 2025
9 of 11 checks passed
@aeneasr aeneasr deleted the revert-3921-aeneasr/fix-docker branch January 11, 2025 12:31
@polarathene
Copy link
Contributor

polarathene commented Jan 11, 2025

@aeneasr might I suggest you take a different merge strategy in future for PRs? You appear to be using the default merge commit which interleaves a PR branches individual commits into the target branch history.


Not only does it bring all the individual commits into the target branch, if the branch is long-lived it'll interleave the commits with those from other PRs merged based on timestamps of the individual commits. Without interactive rebase to clean-up commit history this tends to be even worse to navigate.

Merging this revert for example:

image

I doubt those synchronize workspaces commits provide any value to git blame or the PR itself (EDIT: Nevermind, they were review feedback changes, title was misleading for what they represented).

In projects I maintain my personal preference is a squash merge commit. I leave commit history in the PR that is reviewed, it is very rare that the change warrants tailoring multiple pristine commits for the master branch, so this bundles the merge into a single logical commit with a reference to the PR.

Actually the revert commit history seems to lack a merge commit reference, instead the follow-up PR that brought back the changes with the shared revert commits in it's branch history was merged with the PR ref 🤔


Anyway... just thought it might be worth mentioning as squash merge commits IMO make more sense for merging PRs, far easier to track the scope of changes and related discussions properly via git blame this way too.

@aeneasr
Copy link
Member Author

aeneasr commented Jan 11, 2025

That was an accident because we activated merge queues recently. I reverted the commits, and squash merged the appropriate commit.

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