search

docker / go-metrics

Package for metrics collection in Docker projects
Apache License 2.0
87 stars 32 forks source link

Add CI and vendor deps #26

Closed milosgajdos closed 2 years ago

thaJeztah commented 2 years ago

Why vendor the deps; I think this is only a library repository, and not producing binaries, correct?

thaJeztah commented 2 years ago

(still a bit on the fence on https://github.com/docker/go-metrics/pull/24, as it now makes any dependency REQUIRE to update to the latest version, even though this library may work fine on older versions - minimum requirement selection in go mod were basically designed about the reverse (pick the minimum version you need; consumers may pick a more current version if desired)

milosgajdos commented 2 years ago

Why vendor the deps; I think this is only a library repository, and not producing binaries, correct?

I think it's just a habit. I see no harm in doing that plus the number of deps is low. Can revert, but imo, no harm

thaJeztah commented 2 years ago

It'll definitely make things slower when using GOPROXY=direct

milosgajdos commented 2 years ago

(still a bit on the fence on https://github.com/docker/go-metrics/pull/24, as it now makes any dependency REQUIRE to update to the latest version, even though this library may work fine on older versions - minimum requirement selection in go mod were basically designed about the reverse (pick the minimum version you need; consumers may pick a more current version if desired)

we have to move on from this. If people scream, we can do something about it., but we can't maintain b/w comp to go 1.11. Onwards we go.

thaJeztah commented 2 years ago

but can you explain why this library itself needs an updated version?

thaJeztah commented 2 years ago

plus the number of deps is low.

I don't consider this "low" btw

Screenshot 2022-05-04 at 20 46 23
milosgajdos commented 2 years ago

but can you explain why this library itself needs an updated version?

you mean why we are doing this? yeah, distribution depends on this and it's using half-broken prometheus package causing all kinds of shenanigans

milosgajdos commented 2 years ago

I don't consider this "low" btw

yeah as I said I'm not hell-bent on vendoring here :D

thaJeztah commented 2 years ago

Distribution can use a more current version? (just update go.mod and it picks a newer version)

milosgajdos commented 2 years ago

Distribution can use a more current version? (just update go.mod and it picks a newer version)

prometheus is a transtitive dependency via this module. distirbution depends on go-metrics. Otherwise we would not be touching this repo

thaJeztah commented 2 years ago

But there's no local changes, only go.mod update; updating the go mod in distribution does exactly the same? https://github.com/distribution/distribution/compare/main...thaJeztah:update_deps?expand=1

thaJeztah commented 2 years ago

But there's no local changes, only go.mod update; updating the go mod in distribution does exactly the same? https://github.com/distribution/distribution/compare/main...thaJeztah:update_deps?expand=1

milosgajdos commented 2 years ago

But there's no local changes, only go.mod update; updating the go mod in distribution does exactly the same?

Hmm, interesting! Given I've no way of force-push perms I shall close this PR either way, but when I tried this my go.mod-fu failed me. I've tried this now again and everything seems ok!