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

Remove replace directives by upgrading etcd to v3.6.0-alpha.0. #80

Closed
wants to merge 2 commits into from

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Jul 13, 2022

Summary

The replace directive makes it harder for others to import the packages, since it can
break minimum version selection, causing issues when other packages
import this module.

We're already importing this package else where in sigstore
https://github.com/sigstore/cosign/blob/a7c439a29e96d01afee18918d0466ceb685dd747/go.mod#L267.

Signed-off-by: Billy Lynch [email protected]

Release Note

NONE

Documentation

The replace directive makes it harder for others to import the packages, since it can
break minimum version selection, causing issues when other packages
import this module.

We're already importing this package else where in sigstore
https://github.com/sigstore/cosign/blob/a7c439a29e96d01afee18918d0466ceb685dd747/go.mod#L267.

Signed-off-by: Billy Lynch <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #80 (aabe28c) into main (a05dad8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #80   +/-   ##
=======================================
  Coverage   58.94%   58.94%           
=======================================
  Files          21       21           
  Lines        2097     2097           
=======================================
  Hits         1236     1236           
  Misses        799      799           
  Partials       62       62           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a05dad8...aabe28c. Read the comment docs.

Copy link
Collaborator

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

I’d suggest to avoid using alpha versions. We won’t be able to use the policy controller while it uses alphas.

The replace directive makes it harder for others to import the packages, since it can
break minimum version selection, causing issues when other packages
import this module.

v0.20.0 is the same version we were replacing.

Signed-off-by: Billy Lynch <[email protected]>
@wlynch
Copy link
Member Author

wlynch commented Jul 13, 2022

We won’t be able to use the policy controller while it uses alphas.

Can you explain this a bit more? if this is a hard blocker for policy-controller this might be a time bomb since the next release of cosign will cause this upgrade for etcd. 😬

For the time being, reworked this PR by downgrading the otel deps to the replaced version.

@hectorj2f
Copy link
Collaborator

@wlynch It is not a blocker but security advocates for avoiding using any dependency pointing to an alpha version. That is the problem.

@hectorj2f
Copy link
Collaborator

Do we know when v3.6.0-alpha.0 would become a beta ? I'd say if it is expected to land any time soon, then we could use the alpha version for now. I don't want this to be a time bomb when cosign is released :S.

@wlynch
Copy link
Member Author

wlynch commented Jul 13, 2022

Soon is not looking likely :( etcd-io/etcd#13538 (comment)

I'll go ahead and close this PR - anyone who wants to import this module will just need to copy the replace statements for the time being 😞

I'll follow up on sigstore/cosign#2070 about what we want to do about the cosign's usage of the alpha etcd package.

Thanks!

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.

3 participants