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

Add go.mod and pin dependencies #810

Closed
wants to merge 1 commit into from

Conversation

ashokponkumar
Copy link
Contributor

Signed-off-by: Ashok Pon Kumar ashokponkumar@gmail.com

@vbatts
Copy link
Member

vbatts commented Feb 2, 2021

not a bad idea, but it fails travis CI

@vbatts
Copy link
Member

vbatts commented Jul 9, 2021

@ashokponkumar could you rebase?

@ashokponkumar
Copy link
Contributor Author

@ashokponkumar could you rebase?

rebased.

go.sum Outdated
@@ -0,0 +1,241 @@
cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
Copy link
Member

Choose a reason for hiding this comment

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

this go.sum file does not even need to be checked in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its generally the practise to checkin go.sum too. But if we don't want for this repo, we can remove it. (Ex: https://github.com/kubernetes/kubernetes/blob/master/go.sum)

go.mod Outdated
@@ -0,0 +1,12 @@
module github.com/opencontainers/image-spec
Copy link
Member

Choose a reason for hiding this comment

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

after some discussion on the call, I'm not sure that we want to have a single go.mod at the top level of the project, but instead to have a go.mod in any immediate subdirectory that contains golang source.

The reasoning behind this is: if say ./schema/ brings in a dependency that is wholly not needed if someone only needs to import ./specs-go/, importers need only fetch specs-go/go.mod related deps (which is likely always none).

We just did similar on the distribution-spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I have added for specs-go. Adding to schema to creates some ambiguous import issue.

go: finding module for package github.com/opencontainers/image-spec/specs-go/v1
go: found github.com/opencontainers/image-spec/specs-go/v1 in github.com/opencontainers/image-spec v1.0.1
github.com/opencontainers/image-spec/schema: ambiguous import: found package github.com/opencontainers/image-spec/schema in multiple modules:
	github.com/opencontainers/image-spec/schema (/Users/ashokponkumar/go/src/github.com/ashokponkumar/image-spec/schema)
	github.com/opencontainers/image-spec v1.0.1 (/Users/ashokponkumar/go/pkg/mod/github.com/opencontainers/image-spec@v1.0.1/schema)```

Copy link
Member

Choose a reason for hiding this comment

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

oh dang. Just did some research on this, and it's a go modules mess ... and sounds like what @tianon mentioned during the image-spec grooming call with having to do special git tags also.

I found that the popular import github.com/ugorji/go/codec went through this when they brought in go modules.
ugorji/go#299
They ended up having to add circular referencing go.mod files, based on tag versions, and also to make multiple tags for each of the sub-directories as their own modules https://github.com/ugorji/go/tags

I'm now wondering if this is all worth it, or is it just simpler to keep the go.mod file at the topic level directory like you began with :-\

@opencontainers/image-spec-maintainers thoughts?

(p.s. i'm curious why the we didn't hit this in the distribution-spec?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason the issue is not there in distribution-spec is that, it has only one go module in the repo.

Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
vbatts added a commit to vbatts/oci-image-spec that referenced this pull request Dec 2, 2021
Closes opencontainers#810

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
@vbatts vbatts mentioned this pull request Dec 2, 2021
@vbatts vbatts closed this in #883 Dec 2, 2021
vbatts added a commit that referenced this pull request Dec 2, 2021
@vbatts
Copy link
Member

vbatts commented Dec 2, 2021

thank you @ashokponkumar for your PR! and congrats on your first commit!

@ashokponkumar
Copy link
Contributor Author

Thanks @vbatts for taking it through the process and getting it merged.

sudo-bmitch pushed a commit to sudo-bmitch/image-spec that referenced this pull request Sep 25, 2022
Closes opencontainers#810

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
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.

2 participants