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

migrate to gomodules #100

Merged
merged 2 commits into from
May 6, 2019
Merged

migrate to gomodules #100

merged 2 commits into from
May 6, 2019

Conversation

olegsu
Copy link
Contributor

@olegsu olegsu commented May 4, 2019

Related to #99
Changes have to be done

  1. As @jdolitsky pointed to @thaJeztah Usage as a Go library with Go modules #98 (comment) , docker dependency have to be replaced to direct revision that was used in Gopkg.lock and applied as psuedo-version to avoid go mod downgrade (the format used is vX.Y.(Z+1)-0.yyyymmddhhmmss-abcdefabcdef )
  2. Indirect dependency of rsc.io/letsencrypt have to be replaced with the forked version using by docker/distribution https://github.com/deislabs/oras/blob/master/Gopkg.toml#L22
  3. Remove all vendor rules from Makefile
  4. Update pipelines of Codefresh to work from the cloned working directory instead of under GOPATH

make test passed

fix tag in codefresh yaml

fix tag in codefresh yaml

rename to main_clone

replace inderict dependency rsc.io/letsencrypt with the forked version

update to psuedo version of docker/docker

wip

wip

download modules before releasing
@msftclas
Copy link

msftclas commented May 4, 2019

CLA assistant check
All CLA requirements met.


replace (
github.com/docker/docker => github.com/docker/docker v0.0.0-20190131205458-8a43b7bb99cd
rsc.io/letsencrypt => github.com/dmcgowan/letsencrypt v0.0.0-20160928181947-1847a81d2087

Choose a reason for hiding this comment

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

Looking at the changes that were made in the fork (related to vendoring), perhaps the fork is no longer needed if I read the description of this release: https://github.com/rsc/letsencrypt/releases/tag/v0.0.1 (but probably best to make that change he upstream in the github.com/docker/distribution package first)

/cc @dmcgowan @caervs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to replace it to replace rsc.io/letsencrypt => github.com/rsc/letsencrypt v0.0.1 breaking the tests at github.com/deislabs/oras/pkg/auth/docker

+ go test -v -covermode=atomic -coverprofile=.cover/github.com_deislabs_oras_pkg_auth_docker.cover.out github.com/deislabs/oras/pkg/auth/docker
# rsc.io/letsencrypt
../../../go/pkg/mod/github.com/rsc/letsencrypt@v0.0.1/lets.go:674:41: too many arguments in call to c.ObtainCertificate
	have ([]string, bool, nil, bool)
	want ([]string, bool, crypto.PrivateKey)
FAIL	github.com/deislabs/oras/pkg/auth/docker [build failed]

Choose a reason for hiding this comment

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

Hm, interesting; rsc/letsencrypt@33926fa

looks like that may be due to a different version of https://github.com/go-acme/lego being vendored (see go-acme/lego@01974a9).

Don't think it has top-priority now to get rid of that fork, and it can be done upstream first (in the docker/distribution repository)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, as long as the model gives reproducibility and availability of the modules.

@jdolitsky
Copy link
Contributor

/test

@jdolitsky jdolitsky self-requested a review May 6, 2019 04:27
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.

None yet

4 participants