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

Revendor g/g to 1.26 #290

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Revendor g/g to 1.26 #290

merged 1 commit into from
Jul 13, 2021

Conversation

BeckerMax
Copy link
Contributor

@BeckerMax BeckerMax commented Jul 12, 2021

Co-authored-by: Marco Voelz marco.voelz@sap.com

Special notes for your reviewer:
In the go.mod file we saw after make revendor that the following was removed

github.com/prometheus/client_golang v1.8.0

We are unsure whether to remove the replace directive for the package above.
When removing it the code stays the same only the go.sum and vendor/modules.txt changes to include older versions of that package.
WDYT?

Release note:

Do not trigger a node rollout when switching from `CRI.Name==nil` to `CRI.Name==docker`.

Co-authored-by: Marco Voelz <marco.voelz@sap.com>
@BeckerMax BeckerMax requested review from a team as code owners July 12, 2021 13:31
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 12, 2021
@gardener-robot gardener-robot added needs/review Needs review size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Jul 12, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 12, 2021
@ialidzhikov
Copy link
Member

/test

@testmachinery
Copy link

testmachinery bot commented Jul 13, 2021

Testrun: e2e-rftfq
Workflow: e2e-rftfq-wf
Phase: Succeeded

+---------------------+---------------------+-----------+----------+
|        NAME         |        STEP         |   PHASE   | DURATION |
+---------------------+---------------------+-----------+----------+
| infrastructure-test | infrastructure-test | Succeeded | 9m24s    |
+---------------------+---------------------+-----------+----------+

Copy link
Member

@timebertt timebertt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are unsure whether to remove the replace directive for the package above.

I think you can keep it.

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Jul 13, 2021
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@ialidzhikov ialidzhikov merged commit 35bd157 into gardener:master Jul 13, 2021
@BeckerMax
Copy link
Contributor Author

@timebertt
regarding github.com/prometheus/client_golang v1.8.0

Asked the other way around - can we also remove it? As adapting the replace directive when the controllerruntime package is updated is an additional manual step which can be forgotten.
We tried to remove it and at least we got no error from make revendor - so I assume there is no dependency conflict here, right and the replace directive is not needed.

@timebertt
Copy link
Member

I'm not sure, TBH.

$ go mod edit -dropreplace=github.com/prometheus/client_golang
$ make revendor

leaves me with this diff for go.sum:

+github.com/prometheus/client_golang v0.9.0/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw=
+github.com/prometheus/client_golang v0.9.1/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw=
+github.com/prometheus/client_golang v0.9.2/go.mod h1:OsXs2jCmiKlQ1lTBmv21f2mNfw4xf/QclQDMrYNZzcM=
+github.com/prometheus/client_golang v0.9.3/go.mod h1:/TN21ttK/J9q6uSwhBd54HahCDft0ttaMvbicHlPoso=
+github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5FsnadC4Ky3P0J6CfImo=
+github.com/prometheus/client_golang v1.1.0/go.mod h1:I1FGZT9+L76gKKOs5djB6ezCbFQP1xR9D75/vuwEF3g=
+github.com/prometheus/client_golang v1.3.0/go.mod h1:hJaj2vgQTGQmVCsAACORcieXFeDPbaTKGT+JTgUa3og=
+github.com/prometheus/client_golang v1.7.1/go.mod h1:PY5Wy2awLA44sXw4AOSfFBetzPP4j5+D6mVACh+pe2M=
 github.com/prometheus/client_golang v1.11.0 h1:HNkLOAEQMIDv/K+04rukrLx6ch7msSRwf3/SASFAGtQ=
 github.com/prometheus/client_golang v1.11.0/go.mod h1:Z6t4BnS23TR94PD6BsDNk8yVqroYurpAkEiz0P2BEV0=

so I assume, there must be some transitive dependencies using v0.9.0.
I think, we originally had the replace stmt, because k/k and ks/cr didn't use the same prom version at the time, but this is resolved these days.
However, pinning it to one version shouldn't hurt. But as said, I'm not sure about the exact consequences.

@voelzmo
Copy link
Member

voelzmo commented Jul 15, 2021

We're thinking about removing this replace statement, as it is easy to forget to maintain this (and the implications aren't very clear to us, I think). For example look at https://github.com/gardener/gardener-extension-provider-aws/blob/ab9332a2622350db7cee447aa942a1743a68d8b5/go.mod#L44 where g/g has been updated, but the prometheus/client_golang is still replaced to 1.7.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test Test needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/l Size of pull request is large (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants