Vendor check

[vrothberg]

This PR aims at enforcing always synchronized and up to date dependencies by running make vendor with a follow-up git-status check via papr. This way, we don't have to worry about the state of vendor.conf as the CI will complain if something is out of sync, or if certain packages are not needed anymore in ./vendor.

The idea to do this popped up over at CRI-O but I figured to start here, given the dependencies are already cleaned up :) If desired, I can open similar PRs over at Buildah and Skopeo.

Cc: @cevich @runcom @baude @mheon @rhatdan @mtrmac @TomSweeneyRedHat

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mheon]

Why isn't this wiping out the extra stuff Varlink vendors, I wonder? There are files that we're definitely not using in that vendor tree (using in the main code, at least... it builds a separate binary we use in the build phase), and I don't think we've told vndr that directory should be treated specially

[vrothberg]

make vendor has a whitelist :)

…

On Sun 3. Feb 2019 at 16:13, Matthew Heon ***@***.***> wrote: Why isn't this wiping out the extra stuff Varlink vendors, I wonder? There are files that we're definitely not using in that vendor tree (using in the main code, at least... it builds a separate binary we use in the build phase), and I don't think we've told vndr that directory should be treated specially — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2259 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALI4g0OwMIVeyKNaFitbvlrFGkHYPYedks5vJvyVgaJpZM4agCKJ> .

[cevich]

I would prefer to have new checks like this go into Cirrus, since PAPR is in maintenance mode. I think what you're doing here could happen inside a container, no? Something like:

diff --git a/.cirrus.yml b/.cirrus.yml
index 0efe7380..00927f32 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -110,6 +110,25 @@ gating_task:
         - '/usr/local/bin/entrypoint.sh lint'
 
 
+vendor_check_task:
+
+    depends_on:
+        - "gating"
+
+    env:
+        CIRRUS_WORKING_DIR: "/usr/src/libpod"
+
+    # Runs within Cirrus's "community cluster"
+    container:
+        image: "quay.io/libpod/gate:latest"
+        cpu: 4
+        memory: 12
+
+    gate_script:
+        - '/usr/local/bin/entrypoint.sh vendor'
+        - 'cd /go/src/github.com/containers/libpod && ./hack/tree_status.sh'
+
+
 build_each_commit_task:
 
     depends_on:
@@ -253,6 +272,7 @@ success_task:
 
     depends_on:  # ignores any dependent task conditions
         - "gating"
+        - "vendor_check"
         - "testing"
         - "optional_testing"
         - "cache_images"

(not tested: I think I spelled everything correctly)

[mtrmac]

Yes please; I’ve been frustrated a quite few times by repositories that immediately break when running make vendor and clearly have been in that state for months.

Keeping master branches in vendor.conf and then just not updating them has clearly not worked, so let’s at least be honest about pinning specific git commits (as I suspect this will force us to).

[cevich]

@vrothberg Hmm, one thing just occurred to me: If a contributor fixes a small typo, their PR could be the unlucky one that finds vendor code is out of date. In other words, it may not be fair to place this burden on everyone, all the time.

What do you think , if we limit this check to only running after merge? When the post-merge testing fails, it puts an ugly red mark on the front-page: https://github.com/containers/libpod#library-and-tool-for-running-oci-based-containers-in-pods In theory that means it will get noticed...eventually 😄

[cevich]

(hint: See the build_each_commit_task section for how to limit this for post-merge/master testing)

[vrothberg]

I would prefer to have new checks like this go into Cirrus, since PAPR is in maintenance mode. I think what you're doing here could happen inside a container, no? Something like:

Sounds great! Can we get this check to run only for 1 job and not all to avoid redundant checks?

@vrothberg Hmm, one thing just occurred to me: If a contributor fixes a small typo, their PR could be the unlucky one that finds vendor code is out of date. In other words, it may not be fair to place this burden on everyone, all the time.

I don't think this could happen as we do not vendor branches anymore. The CI will only complain when the code, vendor.conf and the vendor directory are not in sync anymore and I think that's the right thing to do. This way, we can be sure that everything's in line.

To push it a bit further: with this change, we actually protect contributors from cleaning up or dealing with the vendor mess. Unfortunately, it is very common that much more things get changed when running vndr $project. When running make vendor now, only things are changed that the user actually caused.

[cevich]

@vrothberg oohhh, When running make vendor now, only things are changed that the user actually caused. is the key I was missing. Yes the diff I posted will only run one task per PR, no matrix for each platform. It'll run in parallel to the other tasks (except gating) and be fast (since it's in a container).

[rhatdan]

@cevich @vrothberg Is this ready to merge?

[cevich]

@rhatdan Not yet thx for asking.

[cevich]

@vrothberg in fact, if you want, this could be even simpler: Just tack the new commands onto the end of the existing 'gate' task's script section. That will cause your check to gate ALL other testing if it fails. So long as the check is quick, there's no reason for it to live in a separate task. I'm fine either way.

[vrothberg]

@cevich, thanks for your great input! I changed the PR accordingly and also made hack/tree_status.sh print the files that have changed to let us know what has changed.

[cevich]

@vrothberg Hehe, 36-seconds to run 😄 I thought it would take many more minutes. Thanks for writing up what this does! Future maintainers and automation-junkies will appreciate that.

Now that you've got it working, and since it's so fast, what do you think about just including this check in the existing gating task (it's the same environment, and won't add much time)? Also can we put your write up into the main docs?

diff --git a/.cirrus.yml b/.cirrus.yml
index dbd0e8b3..ea961a31 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -109,6 +109,12 @@ gating_task:
         - '/usr/local/bin/entrypoint.sh validate'
         - '/usr/local/bin/entrypoint.sh lint'
 
+    vendor_script:
+        - '/usr/local/bin/entrypoint.sh .install.vndr'
+        - '/usr/local/bin/entrypoint.sh vendor'
+        - 'cd /go/src/github.com/containers/libpod && ./hack/tree_status.sh'
+
+
 build_each_commit_task:
 
     depends_on:
diff --git a/contrib/cirrus/README.md b/contrib/cirrus/README.md
index e175479f..db93c6b2 100644
--- a/contrib/cirrus/README.md
+++ b/contrib/cirrus/README.md
@@ -28,6 +28,10 @@ task (pass or fail) is set based on the exit status of the last script to execut
 4. ``lint``: Execute regular `make lint` to check for any code cruft.
    Should also run for less than a few minutes.
 
+5. ``vendor``: runs `make vendor` followed by `./hack/tree_status.sh` to check whether the
+   git tree is clean.  The reasoning for that is to make sure that the vendor.conf,
+   the code and the vendored packages in ./vendor are in sync at all times.
+
 
 ### ``testing`` Task
 

There, now it's a perfect, polished, glossy-black finish on a race-car, and surely I best stop waxing it or it will never cross any finish-line 😃

[vrothberg]

There, now it's a perfect, polished, glossy-black finish on a race-car, and surely I best stop waxing it or it will never cross any finish-line smiley

Done ✔️

[cevich]

/lgtm

[vrothberg]

WARNING: dependency is not vendored: github.com/onsi/ginkgo

@baude @cevich ... to make this work, we need to vendor gingko and gomega (and their deps) as well instead of go-getting them. We can then build them from the sources in ./vendor.

[cevich]

@vrothberg oh joy of joys. Was just talking about that with @edsantiago the other day. I fear it might be a royal PITA 😢 (the dependencies are many in number from what Ed said).

[edsantiago]

@cevich my comment that day was that it was a mess because ginkgo wasn't vendored in; I think the least-awful approach is to vendor it + deps.

[openshift-merge-robot]

/retest

[vrothberg]

@cevich my comment that day was that it was a mess because ginkgo wasn't vendored in; I think the least-awful approach is to vendor it + deps.

Done 👍

[vrothberg]

This is getting seriously frustrating. Parts of the vendor/github.com/varlink come from different commits. The API has changed which now breaks the build. This PR will ultimately avoid such situations but it shows how easy it is to work around make vendor.

[vrothberg]

The vendored buildah had a very similar issue, so I reverted the breaking commit and cleaned up the varlink stuff.

[vrothberg]

Green 🙏

[rhatdan]

/lgtm