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 in go.mod. #2070

Merged
merged 1 commit into from
Jul 15, 2022
Merged

Remove replace directives in go.mod. #2070

merged 1 commit into from
Jul 15, 2022

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Jul 13, 2022

Summary

As @cpanato pointed out in #2054 (comment), the replace directive introduced in #2054 breaks go install when running outside of the
cosign directory.

$ go install github.com/sigstore/cosign/cmd/cosign@df94451e5581d1354af086f7d1faed9f69b826cf
go: downloading github.com/sigstore/cosign v1.9.1-0.20220707161345-df94451e5581
go: github.com/sigstore/cosign/cmd/cosign@df94451e5581d1354af086f7d1faed9f69b826cf (in github.com/sigstore/[email protected]):
        The go.mod file for the module providing named packages contains one or
        more replace directives. It must not contain directives that would cause
        it to be interpreted differently than if it were the main module.

This swaps the replaces with a go get go.etcd.io/etcd/[email protected], which should generally have the same
effect.

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

Release Note

NONE

Documentation

n/a

The replace directive break `go install` when running outside of the
cosign directory.

```sh
$ go install github.com/sigstore/cosign/cmd/cosign@df94451e5581d1354af086f7d1faed9f69b826cf
go: downloading github.com/sigstore/cosign v1.9.1-0.20220707161345-df94451e5581
go: github.com/sigstore/cosign/cmd/cosign@df94451e5581d1354af086f7d1faed9f69b826cf (in github.com/sigstore/[email protected]):
        The go.mod file for the module providing named packages contains one or
        more replace directives. It must not contain directives that would cause
        it to be interpreted differently than if it were the main module.
```

This swaps the `replaces` with a `go get
go.etcd.io/etcd/[email protected]`, which should generally have the same
effect. Version selection for etcd packages with these changes:

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

wlynch commented Jul 13, 2022

I can't assign people, but cc @cpanato @SantiagoTorres

@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #2070 (1511555) into main (a7c439a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2070   +/-   ##
=======================================
  Coverage   26.33%   26.33%           
=======================================
  Files         129      129           
  Lines        7564     7564           
=======================================
  Hits         1992     1992           
  Misses       5317     5317           
  Partials      255      255           

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 a7c439a...1511555. Read the comment docs.

@wlynch
Copy link
Member Author

wlynch commented Jul 13, 2022

Had some discussion on sigstore/policy-controller#80 - cosign's usage of the alpha etcd API might be problematic for other sigstore projects that want to rely on cosign for verification behavior but do not want to adopt an alpha API. This may make it difficult for downstream projects update past the current release. 😢

We're a bit stuck here because the otel packages made breaking changes from v0 to v1. OPA > v0.35 requires the v1 packages, but etcd <= v3.5.4 (e.g. non-alpha) needs v0.

Any ideas for how we might disentangle these deps? Perhaps we should see if we can break apart the github.com/google/certificate-transparency-go libraries so we can use the inclusion proof checking without the etcd dependency from trillian?

@hectorj2f
Copy link
Contributor

My 2 cents here: if the overall quality of this alpha version is sufficiently high and any breaking change is not affecting the imported functionalities, then it should be fine.

@dlorenc
Copy link
Member

dlorenc commented Jul 14, 2022

I don't care about using alpha stuff, this seems good to me.

@dlorenc dlorenc merged commit e088f46 into sigstore:main Jul 15, 2022
@github-actions github-actions bot added this to the v1.10.0 milestone Jul 15, 2022
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