.gitignore,vendor.conf,Makefile: use vndr to manage dependencies for testing. #240

erikh · 2017-02-21T10:22:20Z

This PR allows us to test against known versions in a vendor.conf provided by
https://github.com/LK4D4/vndr, which is incorporated into the Makefile for
installation and use.

The goal of this PR is to allow us to both test against a specific set of
controllable versions and also refer to this set of versions as a canonical
"this is what we support", without actually forcing the libraries on anyone who
might want to import this package.

runcom · 2017-02-21T10:32:43Z

Makefile

+	go get -u github.com/LK4D4/vndr
+	vndr
+
+test: vndr


we might want to remove test-skopeo? @mtrmac @cyphar wdyt?

At the moment I don't think we have enough (read: any) integration tests for me to say we should drop the only proper integration tests we have. In the future we should drop it IMO.

The original goal of test-skopeo was to stop us from constantly breaking test-skopeo with API changes, without even realizing that we broke it; I think that still very much applies.

(In principle, we could instead have some kind of public API list and just fail tests every time we break the API. But at least the two of us would end up using that test failure to update skopeo, so for us it amounts to the same thing; not sure about other contributors.)

erikh · 2017-02-21T10:36:48Z

I didn't want to jump the gun on that, but would if you wanted.

…

On Tue, Feb 21, 2017 at 2:32 AM, Antonio Murdaca ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Makefile <#240 (comment)>: > @@ -15,8 +15,12 @@ deps: go get -u $(BUILDFLAGS) github.com/golang/lint/golint go get $(BUILDFLAGS) github.com/vbatts/git-validation -test: - @go test $(BUILDFLAGS) -cover ./... +vndr: + go get -u github.com/LK4D4/vndr + vndr + +test: vndr we might want to remove test-skopeo? @mtrmac <https://github.com/mtrmac> @cyphar <https://github.com/cyphar> wdyt? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#240 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ6418mQArBnRLxzGJWRKT1a1ZbSQZks5rer1MgaJpZM4MHIC5> .

erikh · 2017-02-21T10:47:26Z

figures; gopkg.in is busted right now.

erikh · 2017-02-21T12:09:44Z

blocked by containers/skopeo#312 -- @runcom can you help me with this one?

mtrmac · 2017-02-21T19:21:12Z

This primarily seems blocked by the vendor mechanism breaking API compatibility, see distribution/distribution#2130 for a full explanation.

When test-skopeo copies containers/image into skopeo’s vendor subdirectory, it must not copy containers/image/vendor in there as well.

(Being dragged kicking and screaming towards containers/image vendoring anything at all, I would still like it if it were very easy to run tests against the current or latest versions, i.e. using a separate non-default directory and an explicit GOPATH override to run the tests. But I can live with this.)

mtrmac · 2017-02-21T19:25:26Z

Makefile

+	go get -u github.com/LK4D4/vndr
+	vndr
+
+test: deps


Is this strictly necessary? Slowing down the edit-test cycle is not all that attractive, and I currently tend to use go build ./... && make .gitvalidation test validate instead of make all to avoid the deps step.

make all will run deps anyway.

At the very least, the vndr target should be able to use a timestamp so that vndr gets rerun only when vendor.conf is updated.

mtrmac · 2017-02-21T19:26:57Z

Makefile

-	@out="$$(golint ./...)"; \
-	if [ -n "$$(golint ./...)" ]; then \
+	@out="$$(golint $(PACKAGES))"; \
+	if [ -n "$$(golint $(PACKAGES))" ]; then \


Wait, we are running golint twice here? Please replace the second reference with $$out, perhaps as a separate commit.

erikh · 2017-02-22T04:35:25Z

here's a solution to the multi-vendor problem which I think also resolves your concerns about wanting to move away from the vendor directory when you need to.

If the vendor directory is populated at all in the repo, the vndr target will now quietly succeed without running, allowing for improved test times AND the ability to populate your own vendor directory without fear of having it wiped out.

Work for you? I also resolved the golint issues.

mtrmac · 2017-02-22T19:53:39Z

Thanks, this does work for me.

Perhaps it would be possible to do

vendor: vendor.conf
	go get -u github.com/LK4D4/vndr && vndr

(or perhaps there would have to be a separate timestamp file, in case vndr runs but decides not to change anything?) to actually take advantage of make, in particular for a vendor.conf update to cause vndr to be run automatically — but I am perfectly happy with the current Makefile which runs vndr once on initial checkout, and requires manual reruns.

I guess containers/skopeo#304 would end up with decisions how to manage the dependency list, no need to rehash that here.

As for the current test failure:

When test-skopeo copies containers/image into skopeo’s vendor subdirectory, it must not copy containers/image/vendor in there as well.

runcom · 2017-02-23T14:41:00Z

we need this in order to bump image-spec to RC5 and adapt the code for the new ManifestIndex stuff (I'm already working on it but waiting to have these things settled before making a PR)

mtrmac · 2017-02-23T14:47:50Z

Makefile


 # This is not run as part of (make all), but Travis CI does run this.
 # Demonstarting a working version of skopeo (possibly with modified SKOPEO_REPO/SKOPEO_BRANCH, e.g.
 #    make test-skopeo SKOPEO_REPO=runcom/skopeo-1 SKOPEO_BRANCH=oci-3 SUDO=sudo
 # ) is a requirement before merging; note that Travis will only test
 # the master branch of the upstream repo.
-test-skopeo:
+test-skopeo: clean


It is extremely non-obvious why the clean here matters, and it is unintuitive that make test-something modifies the vendored libraries in the current directory.

At the very least this needs a detailed comment explaining why the “clean” here is essential and shouldn’t be removed; but I’d much prefer dropping this dependency, and doing rm -rf $${vendor_path} && cp -r . $${vendor_path} && rm -rf $${vendor_path}/vendor below.

erikh · 2017-02-23T18:41:46Z

that's going to completely destroy vendor every time the tests are run, we want to keep vndr out of the way between runs right?

…

On Thu, Feb 23, 2017 at 6:48 AM, Miloslav Trmač ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In Makefile <#240 (comment)>: > # This is not run as part of (make all), but Travis CI does run this. # Demonstarting a working version of skopeo (possibly with modified SKOPEO_REPO/SKOPEO_BRANCH, e.g. # make test-skopeo SKOPEO_REPO=runcom/skopeo-1 SKOPEO_BRANCH=oci-3 SUDO=sudo # ) is a requirement before merging; note that Travis will only test # the master branch of the upstream repo. -test-skopeo: +test-skopeo: clean It is *extremely* non-obvious why the clean here matters, and it is unintuitive that make test-something modifies the vendored libraries in the current directory. At the very least this needs a detailed comment explaining why the “clean” here is essential and shouldn’t be removed; but I’d much prefer dropping this dependency, and doing rm -rf $${vendor_path} && cp -r . $${vendor_path}*&& rm -rf $${vendor_path}/vendor* below. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#240 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABJ65nCh1OYDPKza_sl4nBr6SI2NXNkks5rfZwsgaJpZM4MHIC5> .

erikh · 2017-02-23T18:42:11Z

ah misunderstood. I'll look into it, not sure why the copy is necessary though.

…

On Thu, Feb 23, 2017 at 10:41 AM, Erik Hollensbe ***@***.***> wrote: that's going to completely destroy vendor every time the tests are run, we want to keep vndr out of the way between runs right? On Thu, Feb 23, 2017 at 6:48 AM, Miloslav Trmač ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > ------------------------------ > > In Makefile > <#240 (comment)>: > > > > # This is not run as part of (make all), but Travis CI does run this. > # Demonstarting a working version of skopeo (possibly with modified SKOPEO_REPO/SKOPEO_BRANCH, e.g. > # make test-skopeo SKOPEO_REPO=runcom/skopeo-1 SKOPEO_BRANCH=oci-3 SUDO=sudo > # ) is a requirement before merging; note that Travis will only test > # the master branch of the upstream repo. > -test-skopeo: > +test-skopeo: clean > > It is *extremely* non-obvious why the clean here matters, and it is > unintuitive that make test-something modifies the vendored libraries in > the current directory. > > At the very least this needs a detailed comment explaining why the > “clean” here is essential and shouldn’t be removed; but I’d much prefer > dropping this dependency, and doing rm -rf $${vendor_path} && cp -r . > $${vendor_path}*&& rm -rf $${vendor_path}/vendor* below. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#240 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABJ65nCh1OYDPKza_sl4nBr6SI2NXNkks5rfZwsgaJpZM4MHIC5> > . >

erikh · 2017-02-23T20:34:30Z

ok. This should be passing now.

…

On Thu, Feb 23, 2017 at 10:42 AM, Erik Hollensbe ***@***.***> wrote: ah misunderstood. I'll look into it, not sure why the copy is necessary though. On Thu, Feb 23, 2017 at 10:41 AM, Erik Hollensbe ***@***.***> wrote: > that's going to completely destroy vendor every time the tests are run, > we want to keep vndr out of the way between runs right? > > On Thu, Feb 23, 2017 at 6:48 AM, Miloslav Trmač ***@***.*** > > wrote: > >> ***@***.**** requested changes on this pull request. >> ------------------------------ >> >> In Makefile >> <#240 (comment)>: >> >> > >> # This is not run as part of (make all), but Travis CI does run this. >> # Demonstarting a working version of skopeo (possibly with modified SKOPEO_REPO/SKOPEO_BRANCH, e.g. >> # make test-skopeo SKOPEO_REPO=runcom/skopeo-1 SKOPEO_BRANCH=oci-3 SUDO=sudo >> # ) is a requirement before merging; note that Travis will only test >> # the master branch of the upstream repo. >> -test-skopeo: >> +test-skopeo: clean >> >> It is *extremely* non-obvious why the clean here matters, and it is >> unintuitive that make test-something modifies the vendored libraries in >> the current directory. >> >> At the very least this needs a detailed comment explaining why the >> “clean” here is essential and shouldn’t be removed; but I’d much prefer >> dropping this dependency, and doing rm -rf $${vendor_path} && cp -r . >> $${vendor_path}*&& rm -rf $${vendor_path}/vendor* below. >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> <#240 (review)>, >> or mute the thread >> <https://github.com/notifications/unsubscribe-auth/AABJ65nCh1OYDPKza_sl4nBr6SI2NXNkks5rfZwsgaJpZM4MHIC5> >> . >> > >

mtrmac

Thanks!

mtrmac · 2017-02-24T21:29:34Z

👍 for pullapprove

runcom · 2017-02-24T22:36:22Z

👍

runcom · 2017-02-24T22:36:44Z

Needs one last rebase

…testing. Signed-off-by: Erik Hollensbe <[email protected]>

Signed-off-by: Erik Hollensbe <[email protected]>

erikh · 2017-02-25T10:37:56Z

it is done

mtrmac · 2017-02-27T18:07:57Z

If the vendor directory is populated at all in the repo, the vndr target will now quietly succeed without running, allowing for improved test times AND the ability to populate your own vendor directory without fear of having it wiped out.

(This has somehow disappeared? Anyway, being dealt with in #242; I am just mildly curious what has happened.)

erikh · 2017-02-27T18:10:12Z

I'm confused too :)

erikh mentioned this pull request Feb 21, 2017

manifest: remove ManifestList type #238

Closed

runcom reviewed Feb 21, 2017

View reviewed changes

erikh force-pushed the temp-vendor branch 2 times, most recently from 6e4c0ff to 702d3ce Compare February 21, 2017 10:44

erikh force-pushed the temp-vendor branch 2 times, most recently from 76f7420 to e5d6ea9 Compare February 21, 2017 10:57

erikh mentioned this pull request Feb 21, 2017

Close with error #236

Merged

mtrmac reviewed Feb 21, 2017

View reviewed changes

mtrmac mentioned this pull request Feb 21, 2017

vendor: update dependencies containers/skopeo#312

Closed

erikh force-pushed the temp-vendor branch from e5d6ea9 to 6f9404d Compare February 22, 2017 04:33

erikh force-pushed the temp-vendor branch from 6f9404d to b54c8bc Compare February 22, 2017 20:31

mtrmac requested changes Feb 23, 2017

View reviewed changes

erikh force-pushed the temp-vendor branch from b54c8bc to 62c2626 Compare February 23, 2017 20:24

mtrmac mentioned this pull request Feb 24, 2017

Provide large file progress reporting hooks to copy operations #219

Merged

mtrmac approved these changes Feb 24, 2017

View reviewed changes

Erik Hollensbe added 2 commits February 25, 2017 02:15

.gitignore,vendor.conf,Makefile: use vndr to manage dependencies for …

0138186

…testing. Signed-off-by: Erik Hollensbe <[email protected]>

README.md: word-wrap

712acc9

Signed-off-by: Erik Hollensbe <[email protected]>

Erik Hollensbe added 3 commits February 25, 2017 02:15

README.md: notice about dependencies

969813f

Signed-off-by: Erik Hollensbe <[email protected]>

Makefile: only golint once

e8f843f

Signed-off-by: Erik Hollensbe <[email protected]>

Makefile: vndr only once; clean target will remove vendor dir

2cf8eaf

Signed-off-by: Erik Hollensbe <[email protected]>

erikh force-pushed the temp-vendor branch from 7c929a4 to 2cf8eaf Compare February 25, 2017 10:16

runcom merged commit b968abc into containers:master Feb 25, 2017

mtrmac mentioned this pull request Feb 27, 2017

makefile: run containers/image unit tests containers/skopeo#308

Closed

enoodle mentioned this pull request Mar 16, 2017

adding vendor directory openshift/image-inspector#20

Closed

cyphar mentioned this pull request Mar 18, 2018

docker: tarfile: improve auto-decompression handling #427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.gitignore,vendor.conf,Makefile: use vndr to manage dependencies for testing. #240

.gitignore,vendor.conf,Makefile: use vndr to manage dependencies for testing. #240

erikh commented Feb 21, 2017

runcom Feb 21, 2017

cyphar Feb 21, 2017

mtrmac Feb 21, 2017

erikh commented Feb 21, 2017 via email

erikh commented Feb 21, 2017

erikh commented Feb 21, 2017

mtrmac commented Feb 21, 2017

mtrmac Feb 21, 2017

mtrmac Feb 21, 2017

erikh commented Feb 22, 2017

mtrmac commented Feb 22, 2017

runcom commented Feb 23, 2017

mtrmac Feb 23, 2017

erikh commented Feb 23, 2017 via email

erikh commented Feb 23, 2017 via email

erikh commented Feb 23, 2017 via email

mtrmac left a comment

mtrmac commented Feb 24, 2017 •

edited by runcom

Loading

runcom commented Feb 24, 2017 •

edited

Loading

runcom commented Feb 24, 2017

erikh commented Feb 25, 2017

mtrmac commented Feb 27, 2017

erikh commented Feb 27, 2017

.gitignore,vendor.conf,Makefile: use vndr to manage dependencies for testing. #240

.gitignore,vendor.conf,Makefile: use vndr to manage dependencies for testing. #240

Conversation

erikh commented Feb 21, 2017

runcom Feb 21, 2017

Choose a reason for hiding this comment

cyphar Feb 21, 2017

Choose a reason for hiding this comment

mtrmac Feb 21, 2017

Choose a reason for hiding this comment

erikh commented Feb 21, 2017 via email

erikh commented Feb 21, 2017

erikh commented Feb 21, 2017

mtrmac commented Feb 21, 2017

mtrmac Feb 21, 2017

Choose a reason for hiding this comment

mtrmac Feb 21, 2017

Choose a reason for hiding this comment

erikh commented Feb 22, 2017

mtrmac commented Feb 22, 2017

runcom commented Feb 23, 2017

mtrmac Feb 23, 2017

Choose a reason for hiding this comment

erikh commented Feb 23, 2017 via email

erikh commented Feb 23, 2017 via email

erikh commented Feb 23, 2017 via email

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Feb 24, 2017 • edited by runcom Loading

runcom commented Feb 24, 2017 • edited Loading

runcom commented Feb 24, 2017

erikh commented Feb 25, 2017

mtrmac commented Feb 27, 2017

erikh commented Feb 27, 2017

mtrmac commented Feb 24, 2017 •

edited by runcom

Loading

runcom commented Feb 24, 2017 •

edited

Loading