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

Purge the root vendor directory #213

Merged

Conversation

timflannagan
Copy link
Contributor

Update the repository layout and avoid commiting the root vendor directory into source. Update the root .gitignore and ignore the vendor directory. For any CI/CD (or utility-esq Makefile targets) avoid referencing the vendor equivalent through go run ... and instead use the go-get-tool from the operator-sdk/kubebuilder world) and pull down that dependency at runtime.

@timflannagan
Copy link
Contributor Author

/cc @joelanford

@timflannagan
Copy link
Contributor Author

I realize we need to also bump this repository to Go 1.17.

@timflannagan
Copy link
Contributor Author

Interesting: "failed to get packages from new commit "HEAD" (5116b59): could not determine GOARCH and Go compiler".

// Generate deepcopy and conversion.
_ "sigs.k8s.io/controller-tools/cmd/controller-gen"
// Manipulate YAML.
_ "github.com/mikefarah/yq/v3"
// Generate embedded files.
_ "github.com/go-bindata/go-bindata/v3/go-bindata"
Copy link
Member

Choose a reason for hiding this comment

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

Are we able to use go-get-tool for go-bindata as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly, this was a bit more complicated that I had anticipated due to how bloated the manifests target is right now, and exporting the PATH variable to point to this repositories' root ./bin directory before running go generate ... wasn't working, and that command was always searching for packages in the vendor directory, despite no GOFLAGS being set in my local environment.

It's possible I'm missing something obvious, but given the o-f/api recent changes that contain massive changelogs due to dependency bumps, maybe we can tackle this in a follow-up, and/or generalize the value we see in the tools.go file going forward throughout the OLM-controller repositories?

Makefile Show resolved Hide resolved
@timflannagan
Copy link
Contributor Author

I'd like to resolve that go-apidiff before merging this to avoid putting this repository in a state where we break that action, and potentially evaluate purging the root tools.go file.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 13, 2022
@timflannagan
Copy link
Contributor Author

Ah, it looks like we don't have the workflow_dispatch event trigger enabled for this repository. Opened a quick PR (#214) for enabling that for the two workflow definitions.

@timflannagan timflannagan force-pushed the remove-vendor branch 4 times, most recently from b3dc359 to e2c4014 Compare January 13, 2022 04:37
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2022
@timflannagan timflannagan force-pushed the remove-vendor branch 2 times, most recently from 7f1da35 to 2622e83 Compare January 18, 2022 23:51
Signed-off-by: timflannagan <timflannagan@gmail.com>
@timflannagan
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2022
@fgiloux
Copy link
Contributor

fgiloux commented Jan 21, 2022

I have seen how annoying the vendor directory is during PR. That said there are good reasons for keeping a copy of the dependencies. A possible mitigation could be to have the vendor directory stored in a separate repository when a new release is cut. What do you think?

@timflannagan
Copy link
Contributor Author

I think the value of the vendor directory has somewhat deteriorated as modules become more stable, and my preference would be to remove the vendor directory from upstream repositories, while maintaining the vendor directory in any downstream environments. That should serve as a nice balance where we can rely on dependencies being available for downstream builds, but help improve the upstream contributor/reviewer experience. Thoughts?

@fgiloux
Copy link
Contributor

fgiloux commented Jan 21, 2022

to remove the vendor directory from upstream repositories, while maintaining the vendor directory in any downstream

Yes that sounds good to me. My main concern was indeed when there is a need to patch a downstream release and the dependency version is nowhere to be found.

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jan 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, timflannagan

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2022
@tylerslaton
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2022
@openshift-merge-robot openshift-merge-robot merged commit 73f2f9e into operator-framework:master Jan 24, 2022
@timflannagan timflannagan deleted the remove-vendor branch January 24, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants