freeipa / freeipa-operator

EXPERIMENTAL kubernetes operator for FreeIPA
31 stars 5 forks source link

Add vendor directory #17

Closed vashirov closed 3 years ago

vashirov commented 3 years ago

I've added /vendor directory and updated go.sum and go.mod (in a separate commit). I'm not sure if additional go modules and sources for the local development are required to be present in go.mod. @avisiedo, what do you think?

Fixes #16

avisiedo commented 3 years ago

@vashirov Sorry mate, I forgot to note down here the thoughts.

If option (1) or (2) are used, keep in mind that the base "builder" image used in Dockerfile should point out the Dockerfile.dev image instead of the basic golang:1.13, so we keep the same environment used in the pipeline when building the container image for the operator.

Personally I prefer to remove the /vendordirectory so that we keep the repository with only the source that concern to the freeipa-operator, but it depends on the circumstances if it were possible the first two points.

In relation to the lint errors, as you commented, the change in the lint.sh script to ignore the /vendor directory would fix the situation. Currently it exists a mechanism to ignore specific files (/devel/lint.ignore), but that was created for punctual files, not this scenario. I would point out two modifications:

About the current state of go.mod and go.sum, I have built the image and the binary from the workstation with no issues,

make
make container-build

so it is fine the current state; the more we advances in the future the more modules we will need to add, but we will know then :)

avisiedo commented 3 years ago

I forgot one thing. Please add a reference to the openshift-ci pipeline manifest in DEVOPS.md file, so it is referenced from this repository. Nice stuff the openshift-ci :smiley:

vashirov commented 3 years ago

IIUC, the main concern is that vendor/ pollutes the repo with non-related files and it has some security implications in case one of the dependencies is vulnerable and needs an update.

The former looks like a common practice these days after Go introduced modules. For example, openshift-installer has these guidelines for developers regarding Go: https://github.com/openshift/installer/blob/master/docs/dev/dependencies.md#go

The latter can be addressed by dependabot which added support for go mod tidy and go mod vendor. And I think one of the reasons of vendoring is to avoid supply-chain attacks and left-pad incidents, since the whole dependency chain is now stored in one place.

Maintaining another container image for the build is an additional cost. It will also differentiate the way we build things locally vs. in CI.

Regarding the linter error, I've pushed another commit to add is-in-vendor function to ignore vendored files from linting. As for the DEVOPS.md file, I'd like to update it once the PR for openshift-release is merged. There are still some outstanding issues there that I need to fix.