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.

Sign up for GitHub

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

Jump to bottom

chore: remove vendor directory #739

Merged
merged 4 commits into from
Jul 24, 2024
Merged

chore: remove vendor directory #739

merged 4 commits into from
Jul 24, 2024

Conversation

tobiasgiese
Copy link
Contributor

Switched to Go modules for dependency management. This simplifies our workflow and reduces repo size.

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

adrianchiris
adrianchiris approved these changes Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

if CI is green im on board with that

@adrianchiris
Copy link
Collaborator

/test-all

@bn222
Copy link
Collaborator

bn222 commented Jul 17, 2024

if CI is green, I'm also on board with this

@tobiasgiese
Copy link
Contributor Author

tobiasgiese commented Jul 17, 2024 •
edited
Loading

Having issues with controller-gen

go: gopkg.in/[email protected]: missing go.sum entry for go.mod file

Added gopkg.in/check.v1 to hack/tools.go already but with no lock. Also the exact version is in the go.sum already.

@tobiasgiese tobiasgiese force-pushed the remove-vendor branch 2 times, most recently from 3f034e9 to 9008124 Compare July 17, 2024 15:30
@tobiasgiese
Copy link
Contributor Author

tobiasgiese commented Jul 17, 2024 •
edited
Loading

@adrianchiris @bn222 FYI - had to add export GOPATH=$(shell go env GOPATH) to the Makefile, otherwise the GH runners downloaded all the deps to the repos go/.
Also removed the obsolete vendor tests in f81700e

Edit: test k8s failed because of flakiness during cert-manager apply from remote

@adrianchiris
Copy link
Collaborator

/test-all

@tobiasgiese tobiasgiese force-pushed the remove-vendor branch 2 times, most recently from d7d23cf to 4313ca3 Compare July 18, 2024 07:56
@tobiasgiese tobiasgiese force-pushed the remove-vendor branch 2 times, most recently from cd4c50a to d4dc47c Compare July 18, 2024 08:15
@adrianchiris
Copy link
Collaborator

/test-all

3 similar comments
@tobiasgiese
Copy link
Contributor Author

/test-all

@tobiasgiese
Copy link
Contributor Author

/test-all

@tobiasgiese
Copy link
Contributor Author

/test-all

@tobiasgiese
Copy link
Contributor Author

/test-all

1 similar comment
@tobiasgiese
Copy link
Contributor Author

/test-all

@zeeke
Copy link
Member

zeeke commented Jul 18, 2024

hi @tobiasgiese , end-to-end lanes are failing because of:

running tests
+ which ginkgo
which: no ginkgo in (/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator:/root/opr-k8s2-1/data/_tool/go/1.22.4/x64/bin:/root/.local/bin:/root/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/usr/local/go/bin:/usr/local/bin:/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator/bin)
+ '[' 1 -ne 0 ']'
++ mktemp -d
+ GINKGO_TMP_DIR=/tmp/tmp.ADYS4SOl1B
+ cd /tmp/tmp.ADYS4SOl1B
+ go mod init tmp
go: creating new go.mod: module tmp
+ go install -mod=readonly github.com/onsi/ginkgo/v2/[email protected]
go: go: module cache not found: neither GOMODCACHE nor GOPATH is set
+ rm -rf /tmp/tmp.ADYS4SOl1B
+ echo 'Downloading ginkgo tool'
Downloading ginkgo tool
+ cd -
/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator
+ GOPATH=/go
+ JUNIT_OUTPUT=/tmp/artifacts
+ export PATH=/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator:/root/opr-k8s2-1/data/_tool/go/1.22.4/x64/bin:/root/.local/bin:/root/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/usr/local/go/bin:/usr/local/bin:/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator/bin:/go/bin
+ PATH=/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator:/root/opr-k8s2-1/data/_tool/go/1.22.4/x64/bin:/root/.local/bin:/root/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/usr/local/go/bin:/usr/local/bin:/root/opr-k8s2-1/data/sriov-network-operator/sriov-network-operator/bin:/go/bin
+ ginkgo -output-dir=/tmp/artifacts --junit-report unit_report.xml -v ./test/validation -- -report=/tmp/artifacts
./hack/run-e2e-conformance.sh: line 21: ginkgo: command not found
failed, retrying
running tests

Would you mind adding the following changes (or anything similar) to this PR?
dabec26

@tobiasgiese tobiasgiese force-pushed the remove-vendor branch 2 times, most recently from 873bc24 to dc91cac Compare July 18, 2024 15:39
@tobiasgiese
Copy link
Contributor Author

I think the issue is that the GH Actions runner has no real home directory. Also go env GOPATH is empty. Maybe the issue are the srirov runners?

@tobiasgiese
Copy link
Contributor Author 

❯ go env GOPATH
/Users/tgiese/go/
❯ echo $GOPATH
/Users/tgiese/go/
❯ unset GOPATH
❯ go env GOPATH
/Users/tgiese/go
❯ export HOME=
❯ go env GOPATH

❯

Seems like the issue is really the empty $HOME

zeeke
zeeke reviewed Jul 18, 2024
View reviewed changes
Makefile Outdated
@@ -27,7 +27,7 @@ WEBHOOK_IMAGE_TAG?=$(IMAGE_REPO)/$(APP_NAME)-webhook:latest
MAIN_PKG=cmd/manager/main.go
export NAMESPACE?=openshift-sriov-network-operator
export WATCH_NAMESPACE?=openshift-sriov-network-operator
export GOFLAGS+=-mod=vendor
export GOPATH=$(shell go env GOPATH)
Copy link
Member

Choose a reason for hiding this comment

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

can you change this to:

export HOME?=$(PWD)

also, can you split this commit that files deletions are separated to other file changes? It It makes very hard to browse changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good idea! 👍🏻
Sure, let me split it

Copy link
Contributor Author

@tobiasgiese tobiasgiese Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The initial idea why I changed this line in the removal commit was that each commit is working. But I can separate them for sure

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's a good principle. But in GH it makes reviews pretty impossible 😅

@tobiasgiese
Copy link
Contributor Author

/test-e2e-nvidia-all

tobiasgiese and others added 3 commits July 22, 2024 08:39
@tobiasgiese @souleb @killianmuldoon
chore: remove vendor directory
faf1ec2 
Switched to Go modules for dependency management. This simplifies our workflow and reduces repo size.

Signed-off-by: Tobias Giese <[email protected]>
Co-authored-by: Soule BA <[email protected]>
Co-authored-by: killianmuldoon <[email protected]>
@tobiasgiese @zeeke
chore: use current directory as HOME if HOME is not set
e0fb813 
We have issue with GOPATH if no HOME env is set.
To fix this we want to make the current directory as our new home.

Signed-off-by: Tobias Giese <[email protected]>
Co-authored-by: Andrea Panattoni <[email protected]>
@tobiasgiese
chore: update github action to check only go mod tidy
00f8a61 
Signed-off-by: Tobias Giese <[email protected]>
@tobiasgiese
Copy link
Contributor Author

/test-e2e-nvidia-all

1 similar comment
@tobiasgiese
Copy link
Contributor Author

/test-e2e-nvidia-all

@tobiasgiese
Copy link
Contributor Author

/test-e2e-nvidia-all

@tobiasgiese
Copy link
Contributor Author

/test-e2e-nvidia-all
was low on disk space, added 50G

@tobiasgiese
Copy link
Contributor Author

/test-all

@tobiasgiese @zeeke
e2e: Install ginkgo via Makefile
25585da 
Signed-off-by: Tobias Giese <[email protected]>
Co-authored-by: Andrea Panattoni <[email protected]>
@tobiasgiese
Copy link
Contributor Author

/test-e2e-nvidia-all

all tests are running now 👍🏻

@adrianchiris
Copy link
Collaborator

thanks for working on this one @tobiasgiese !!
@zeeke PTAL, lets merge this one if you are happy with it.

@adrianchiris
Copy link
Collaborator

punching it ! 🚀

@adrianchiris adrianchiris merged commit ee40683 into k8snetworkplumbingwg:master Jul 24, 2024
12 checks passed
@tobiasgiese tobiasgiese deleted the remove-vendor branch July 24, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eoghan1232 Eoghan1232 Eoghan1232 approved these changes

@adrianchiris adrianchiris adrianchiris approved these changes

@zeeke zeeke Awaiting requested review from zeeke

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tobiasgiese @adrianchiris @bn222 @zeeke @Eoghan1232