Remove vendor dir #1085

dtfranz · 2023-04-07T18:13:27Z

Description of the change:
Removes the vendor directory from source control and removes the need for it during builds so we don't have to keep re-populating it. Also removes a couple of unused dockerfiles which relied on the vendor directory's presence.

Upgrades to actions/setup-go@v4 which caches the go mod packages by default.

Motivation for the change:
Makes the process of downstreaming easier and speeds up builds since we don't have to re-download the vendor directory and run setup-go fresh on each run.

codecov · 2023-04-07T18:20:35Z

Codecov Report

Merging #1085 (b7f1a36) into master (0aebc3c) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head b7f1a36 differs from pull request most recent head 69d4237. Consider uploading reports for the commit 69d4237 to get more accurate results

@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
+ Coverage   52.60%   52.62%   +0.02%     
==========================================
  Files         107      107              
  Lines        9389     9389              
==========================================
+ Hits         4939     4941       +2     
+ Misses       3523     3521       -2     
  Partials      927      927

see 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dtfranz · 2023-04-07T18:48:49Z

Curiously, the goreleaser / build-windows job gets a different hash from the go.sum file. Anybody know why that might be?

.github/workflows/unit.yaml

Makefile

grokspawn · 2023-04-07T19:04:53Z

Curiously, the goreleaser / build-windows job gets a different hash from the go.sum file. Anybody know why that might be?

Is the hash calc OS/ARCH-dependent?

grokspawn · 2023-04-07T19:07:41Z

Completed a review pass. While the comments I left are more of the question/nit variety, I'm left feeling like I'd like to see some proof of concept of this before I'm comfortable merging it.
For e.g. all the build-${OS} actions currently fail to use the cache, missing uniformly.

dtfranz · 2023-04-07T20:43:46Z

Curiously, the goreleaser / build-windows job gets a different hash from the go.sum file. Anybody know why that might be?

Is the hash calc OS/ARCH-dependent?

Looks as though it is, based on the docs. So for our case, we would have a version of the cache for each OS.

Completed a review pass. While the comments I left are more of the question/nit variety, I'm left feeling like I'd like to see some proof of concept of this before I'm comfortable merging it. For e.g. all the build-${OS} actions currently fail to use the cache, missing uniformly.

That's understandable, and I'm happy to prove this out through another project. It should be expected however that the first run of each build action would fail to find a cache, since all of them ran simultaneously with no previous existing cache (setup-go did not use a cache by default until v4). On subsequent re-runs however, all three build-${OS} actions succeeded in using the previous cache here.

everettraven

Changes make sense and look good to me. Holding on any labels to give others a shot to review

grokspawn · 2023-04-11T20:55:56Z

On subsequent re-runs however, all three build-${OS} actions succeeded in using the previous cache here.

That's what I was looking for. Thanks!

grokspawn · 2023-04-11T20:56:38Z

/approve

joelanford · 2023-04-11T21:37:37Z

Re: os-specific cache.

Here's the suggested setup for go modules, which seems to account for that difference: https://github.com/actions/cache/blob/main/examples.md#go---modules

(I'm afraid to click the diff link, so not sure if this is what you already have)

dtfranz · 2023-04-11T21:51:57Z

Re: os-specific cache.

Here's the suggested setup for go modules, which seems to account for that difference: https://github.com/actions/cache/blob/main/examples.md#go---modules

(I'm afraid to click the diff link, so not sure if this is what you already have)

I understand your fears 😄 I'll just paste the addition here. It's essentially the same except that it only targets the vendor dir in this case:

      ...
      - id: cache-vendor
        uses: actions/cache@v3
        env:
          cache-name: cache-vendor-dir
        with:
          path: ./vendor
          key: ${{ runner.os }}-go-vendor-${{ hashFiles('**/go.sum') }}
          restore-keys: |
            ${{ runner.os }}-go-vendor
      - if: ${{ steps.cache-vendor.outputs.cache-hit != 'true' }}
        name: Run make vendor
        run: make vendor
      ...

We can add more paths to the caching in the future if you think it's worthwhile.

m1kola

It seems like our job restores & saves cache 2 times: as part of actions/setup-go and as part of actions/cache:

Do we need both?

Documentation for actions/setup-go says that it has a built-in functionality for caching and restoring go modules based on go.sum. And it seems to be enabled by default in v4.

I haven't looked into this properly, but I assume that once cache is restored go mod vendor just copies everything from restored cache into vendor dir. Should give us aproximately the same results as caching & restoring vendor directory.

Is there any reason why we can not rely on cache from actions/setup-go?

tmshort · 2023-04-14T14:05:25Z

Makefile

@@ -35,18 +35,20 @@ endif
 .PHONY: all
 all: clean test build

-$(CMDS):
+$(CMDS): vendor


Not sure why we are adding the vendor dependency here? Which seems to eventually run go mod vendor to fill/create the vendor directory (which is .gitignored)? That seems like quite a slowdown; especailly for those who run git clean -fdxq a lot.

The builds for this repo are all dependent on the presence of the vendor directory and will fail without it. Running make vendor again when the vendor dir is already there is essentially a no-op since it won't need to re-download them. From my testing go mod vendor will not re-download the entire folder every time; it will first grab them from ~/go/pkg/mod which is a very fast operation. The presence of this folder is also a requirement for the downstream repos, since everything the project is built from needs to exist in the repo, or at least that's my understanding of why.

I would argue this is still vendoring; this PR just gets rid of the directory and the build step has to recreate it each time...

I guess my point is, if everything is already in the GOPATH, why are we copying it over via go mod vendor?

I agree with Todd. I think there is no benefit in copying into vendor since we have local cache for modules.

there is essentially a no-op since it won't need to re-download them

It is a no-op on network, but there will still be some IO on a disk. I think it is not critical, but would be nice to get rid of it if it is not a big effort.

The builds for this repo are all dependent on the presence of the vendor directory and will fail without it.

I think this is because our Makefile has GOFLAGS="-mod=vendor" which is forcing go build to use vendor dir.

Try running this in master:

sed -i 's/GO := GOFLAGS="-mod=vendor" go/GO := go/g' ./Makefile rm -r ./vendor go mod download make build

Even go mod download might not be necessary. I think go build without -mod=vendor will be to download modules (if not present) and build.

I agree with you both here and would prefer to not have the vendor/ dir at all. But I do want to ask the following, not as a challenge but a genuine question as somebody still not terribly familiar with up/down-streaming processes: If we change the build process for the upstream repo to stop using vendor/, will that not make the downstream cherry-picking process more complicated? Since the downstream repo must use the vendor/ dir for the build (somebody correct me if this is wrong), then aren't we beginning to create a significant divergence between the up and downstream repos? Or is this not what you're recommending?

I'm also not super familiar with downstreaming, but I'm happy to look into it together on next week.

After looking at other upstream+downstream repos I've realized that having differences in build process is really not an issue at all. I've addressed both your concerns and removed all requirements on the vendor/ directory in this PR. Obviously these changes won't go to the downstream repo but it should make the process of downstreaming easier without the directory here.

The vendor directory doesn't exist in the downstream staging directory. But there is a root-level vendor.

tmshort · 2023-04-14T14:22:45Z

My concern here is that all this does is get rid of the vendor directory; building recreates the directory. It should be possible to do this without recreating the vendor directory.

dtfranz · 2023-04-14T17:28:48Z

Is there any reason why we can not rely on cache from actions/setup-go?

I'm glad you brought this up; my understanding was that go mod vendor was not covered by this action, which is partially true. Running the setup-go action will not give you the vendor directory, which is why I had the additional cache action setup. However, you were still correct that the additional cache action is unnecessary because setup-go will still download or restore the required packages from cache, then when go mod vendor is run it will pull those packages locally from ~/go/pkg/mod instead of downloading them again. I've updated the branch to simply use setup-go now.

dtfranz · 2023-04-14T17:51:30Z

My concern here is that all this does is get rid of the vendor directory; building recreates the directory. It should be possible to do this without recreating the vendor directory.

The goal of this task was to remove the vendor directory and recreate it from cache when the github actions are run. I think that reworking the build process to remove the need for the vendor directory would be out of scope of the tasks intended purpose; primarily we just wanted to reduce the number of merge conflicts we get from downstreaming with the vendor folder inside source control. I personally don't see any issue with frequent re-runs of go mod vendor, particularly because it won't actually need to download anything at all in the vast majority of cases.

That being said though if there was something else you were getting at here that I misunderstood please let me know!

tmshort · 2023-04-14T17:53:14Z

That being said though if there was something else you were getting at here that I misunderstood please let me know!

No, that's basically it. If we want to get rid of vendoring, there's more to do.

m1kola · 2023-04-14T18:06:58Z

Makefile

@@ -35,18 +35,20 @@ endif
 .PHONY: all
 all: clean test build

-$(CMDS):
+$(CMDS): vendor


I agree with Todd. I think there is no benefit in copying into vendor since we have local cache for modules.

there is essentially a no-op since it won't need to re-download them

It is a no-op on network, but there will still be some IO on a disk. I think it is not critical, but would be nice to get rid of it if it is not a big effort.

The builds for this repo are all dependent on the presence of the vendor directory and will fail without it.

I think this is because our Makefile has GOFLAGS="-mod=vendor" which is forcing go build to use vendor dir.

Try running this in master:

sed -i 's/GO := GOFLAGS="-mod=vendor" go/GO := go/g' ./Makefile rm -r ./vendor go mod download make build

Even go mod download might not be necessary. I think go build without -mod=vendor will be to download modules (if not present) and build.

m1kola · 2023-04-14T18:08:07Z

.github/workflows/goreleaser.yaml

+      - id: cache-vendor
+        uses: actions/cache@v3
+        env:
+          cache-name: cache-vendor-dir
+        with:
+          path: ./vendor
+          key: go-vendor-${{ hashFiles('go.sum') }}
+          enableCrossOsArchive: true
+      - if: ${{ steps.cache-vendor.outputs.cache-hit != 'true' }}
+        name: Run make vendor
+        run: make vendor


Looks like leftovers in this file?

Yes - thanks for noticing!

Removes the vendor directory from source control and attempts to cache the vendor dir when github actions are run. Also removes a couple of Dockerfiles which are no longer used and relied on the vendor directory. Signed-off-by: dtfranz <dfranz@redhat.com>

tmshort

No more vendor!

tmshort · 2023-04-20T17:29:37Z

/lgtm

openshift-ci · 2023-04-20T17:29:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtfranz, everettraven, grokspawn, tmshort

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~~ [grokspawn]

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2023

openshift-ci bot requested review from grokspawn and theishshah April 7, 2023 18:14

grokspawn reviewed Apr 7, 2023

View reviewed changes

.github/workflows/unit.yaml Outdated Show resolved Hide resolved

grokspawn reviewed Apr 7, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

dtfranz force-pushed the remove-vendor-dir branch 4 times, most recently from 0ab0da8 to 0c03c57 Compare April 11, 2023 20:26

dtfranz changed the title ~~WIP: Remove vendor dir~~ Remove vendor dir Apr 11, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2023

everettraven approved these changes Apr 11, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2023

m1kola requested changes Apr 12, 2023

View reviewed changes

tmshort reviewed Apr 14, 2023

View reviewed changes

dtfranz force-pushed the remove-vendor-dir branch from 0c03c57 to 3e8b55c Compare April 14, 2023 17:23

m1kola reviewed Apr 14, 2023

View reviewed changes

dtfranz force-pushed the remove-vendor-dir branch from 3e8b55c to 36d1bd1 Compare April 14, 2023 18:16

m1kola self-requested a review April 14, 2023 22:50

Remove vendor dir

69d4237

Removes the vendor directory from source control and attempts to cache the vendor dir when github actions are run. Also removes a couple of Dockerfiles which are no longer used and relied on the vendor directory. Signed-off-by: dtfranz <dfranz@redhat.com>

dtfranz force-pushed the remove-vendor-dir branch from 36d1bd1 to 69d4237 Compare April 18, 2023 20:59

tmshort approved these changes Apr 20, 2023

View reviewed changes

openshift-ci bot assigned tmshort Apr 20, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2023

openshift-ci bot merged commit 45f491b into operator-framework:master Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove vendor dir #1085

Remove vendor dir #1085

dtfranz commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

dtfranz commented Apr 7, 2023

grokspawn commented Apr 7, 2023

grokspawn commented Apr 7, 2023

dtfranz commented Apr 7, 2023 •

edited

Loading

everettraven left a comment •

edited

Loading

grokspawn commented Apr 11, 2023

grokspawn commented Apr 11, 2023

joelanford commented Apr 11, 2023 •

edited

Loading

dtfranz commented Apr 11, 2023

m1kola left a comment •

edited

Loading

tmshort Apr 14, 2023

dtfranz Apr 14, 2023

tmshort Apr 14, 2023

tmshort Apr 14, 2023 •

edited

Loading

m1kola Apr 14, 2023 •

edited

Loading

dtfranz Apr 14, 2023

m1kola Apr 14, 2023

dtfranz Apr 18, 2023

tmshort Apr 19, 2023

tmshort commented Apr 14, 2023

dtfranz commented Apr 14, 2023

dtfranz commented Apr 14, 2023

tmshort commented Apr 14, 2023

m1kola Apr 14, 2023 •

edited

Loading

m1kola Apr 14, 2023

dtfranz Apr 14, 2023

tmshort left a comment

tmshort commented Apr 20, 2023

openshift-ci bot commented Apr 20, 2023

Remove vendor dir #1085

Remove vendor dir #1085

Conversation

dtfranz commented Apr 7, 2023 • edited Loading

codecov bot commented Apr 7, 2023 • edited Loading

Codecov Report

dtfranz commented Apr 7, 2023

grokspawn commented Apr 7, 2023

grokspawn commented Apr 7, 2023

dtfranz commented Apr 7, 2023 • edited Loading

everettraven left a comment • edited Loading

Choose a reason for hiding this comment

grokspawn commented Apr 11, 2023

grokspawn commented Apr 11, 2023

joelanford commented Apr 11, 2023 • edited Loading

dtfranz commented Apr 11, 2023

m1kola left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

m1kola Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Apr 14, 2023

dtfranz commented Apr 14, 2023

dtfranz commented Apr 14, 2023

tmshort commented Apr 14, 2023

m1kola Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort left a comment

Choose a reason for hiding this comment

tmshort commented Apr 20, 2023

openshift-ci bot commented Apr 20, 2023

dtfranz commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

dtfranz commented Apr 7, 2023 •

edited

Loading

everettraven left a comment •

edited

Loading

joelanford commented Apr 11, 2023 •

edited

Loading

m1kola left a comment •

edited

Loading

tmshort Apr 14, 2023 •

edited

Loading

m1kola Apr 14, 2023 •

edited

Loading

m1kola Apr 14, 2023 •

edited

Loading