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

go.*: Remove the stale github.com/Sirupsen/logrus replace directive #9927

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

timflannagan
Copy link
Member

Description

A quick maintenance PR for removing a stale replace directive. This was previously needed way back in time when the original logrus repo was renamed. See sirupsen/logrus#570 for more details.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@github-actions github-actions bot added keep pr updated signals bulldozer to keep pr up to date with base branch work in progress signals bulldozer to keep pr open (don't auto-merge) labels Aug 20, 2024
Copy link

Visit the preview URL for this PR (updated for commit b333f45):

https://gloo-edge--pr9927-chore-remove-stale-r-idggo5dk.web.app

(expires Tue, 27 Aug 2024 22:42:55 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

@Implausiblyfun
Copy link

can you please attach the go mod graph before and after to show that this replace is not in affect?

@timflannagan
Copy link
Member Author

@Implausiblyfun Just to quickly clarify: for the mod graph, what do you suggest? Running go mod why ... locally and pasting the output or did you have something else in mind? In either case, I'm happy to do that tomorrow and paste the output as a comment when I get back to my comp.

With that said, I'm now wondering whether the current evidence, which is running go mod tidy doesn't result in any go.sum changes, and cating the go.sum file itself locally doesn't contain this directive could be concluded as sufficient evidence? AFAIK, this used to be a problem when the repository was renamed, but indirect repositories that consume this library hadn't updated their go.mod files, and it was causing wide spread conflicts. Since then, the ecosystem has adjusted as we can remove the replace pin to get some short term maintenance wins.

@nfuden
Copy link
Contributor

nfuden commented Aug 21, 2024

yeah just something quick to show that the replace has no effect therefore the removal is a net win.

@timflannagan
Copy link
Member Author

@nfuden I ran this from a different branch, but here's the why output:

$ dev :: /work/gloo ‹chore/update-contributing› » go mod why github.com/Sirupsen/logrus      
# github.com/Sirupsen/logrus
(main module does not need package github.com/Sirupsen/logrus)

I think this is sufficient along with go mod tidy producing no go.sum changes, and the go.sum file doesn't contain this entry either.

@timflannagan timflannagan enabled auto-merge (squash) August 21, 2024 16:58
@timflannagan timflannagan removed the work in progress signals bulldozer to keep pr open (don't auto-merge) label Aug 21, 2024
@timflannagan timflannagan merged commit 3f8a060 into main Aug 21, 2024
18 checks passed
@timflannagan timflannagan deleted the chore/remove-stale-replace-pin branch August 21, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants