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

feat(dependency): add diff package to common #648

Closed
wants to merge 3 commits into from

Conversation

dongjiang1989
Copy link

@dongjiang1989 dongjiang1989 commented Jun 6, 2024

Add diff package to common for client-golang, prometheus and alertmanager.

issue: prometheus/client_golang#1490
pr for client-glang: prometheus/client_golang#1499

@SuperQ
Copy link
Member

SuperQ commented Jun 19, 2024

Wait, no, we should not be copy-pasting code like this.

Fixing an EoL package by replacing it with something not EoL is just fine. There's no need to fully remove a dependency.

CC @kakkoyun

@kakkoyun
Copy link
Member

Wait, no, we should not be copy-pasting code like this.

Fixing an EoL package by replacing it with something not EoL is just fine. There's no need to fully remove a dependency.

CC @kakkoyun

We did this in client_golang to reduce the external dependencies. And it is small enough and only used int the tests. I never thought it would be needed elsewhere.

That being said, I'd be more than happy to accept prometheus/client_golang#1539.

@SuperQ
Copy link
Member

SuperQ commented Jun 20, 2024

That was incorrectly done in client_golang. The request was to replace the dependency that was abandoned, not reduce it.

@kakkoyun
Copy link
Member

That was incorrectly done in client_golang. The request was to replace the dependency that was abandoned, not reduce it.

I'm well aware of the request. We wanted to kill two birds with one stone.

@ArthurSens
Copy link
Member

Add diff package to common for client-golang, prometheus and alertmanager.

I'm not sure if I'm missing something, but does prometheus and alertmanager really use this same logic? I'm aware that client_golang uses this for testing, but maybe we could avoid adding it to common if it's only for client_golang.

Now that prometheus/client_golang#1539 was accepted it's even less important to have this here from my point of view 🤔

@SuperQ
Copy link
Member

SuperQ commented Jun 20, 2024

IMO, we don't want to get into the habit of in-lining Go dependencies. This sets a hard to deal with policy long-term. What is the limit for an inline? How do we keep inline code up-to-date?

@SuperQ SuperQ closed this Jun 20, 2024
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.

4 participants