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

initial draft for pulumi supporting mulitple providers #26

Closed
wants to merge 4 commits into from

Conversation

adrianriobo
Copy link
Contributor

@adrianriobo adrianriobo commented Jan 18, 2023

Fix #8

@lmilbaum
Copy link
Contributor

Would you mind checking if there is an option not to vendor those dependencies but to pull them during runtime?

@adrianriobo
Copy link
Contributor Author

adrianriobo commented Jan 19, 2023

Hi @lmilbaum vendor folder is not required but I think it is considered a good practice with golang. Each point of view about it could be arguable ...you can see it just with a quick search on the topic.

If you check the size of the project it is not big, as the vendor folder is holding the source code not any compiled bits required on runtime, actually the vendor is used on build time and if vendor is not included the dependencies are directly downloaded.So thinking long term if we decide to go with golang structure the vendor would probably be included here.

Also after build the crc-cloud binary is all self contained (from a code dependencies point of view) it is a single "portable" executable

@lmilbaum
Copy link
Contributor

My previous comment is a bit inaccurate. What I meant is to download the dependencies during build time if they weren't downloaded before. go supports a lock file mechanism where you list your dependencies with pinned versions. If I am not mistaken then the files are: go.mod and go.sum.
Renovate knows how to process those files and submit PRs with updated versions.
It is not a good engineering practice to vendor third party dependencies. Just my 2 cents on that.

@adrianriobo
Copy link
Contributor Author

adrianriobo commented Jan 20, 2023

Hey @lmilbaum yeah I mean I came from java world so you just use you pom to define dependencies ..vendoring as you mention could be considered as an anti-pattern, but in regard to golang (I have been coding with it just couple of years and vendoring here seems a debatable option)

If you check some repos podman, kubernetes, tekton ... all of them hold the vendor folder, some other do not, like asw-sdk, terraform so as I said before at least on go this is something arguable

On the other hand I was no aware of this Renovate I guess you mean this renovate-go so thx it seems super useful to at least point you to newer versions with PRs. But as I can see from doc this is also supports vendoring aswell. Also checking some of those repos I share I found some of them use something similar within gh actions with [dependabot] so really appreciate your comment to put me in this direction 👏

@cfergeau
Copy link

The vendor dir is managed by go through go mod vendor and is expected to matches the content of the go.mod file, if they are not in sync, that's a serious bug.
Tools like go mod tidy or https://github.com/crc-org/crc/blob/main/verify-vendor.sh can be used for stricter checks.
The fact that there's a vendor directory does not even mean it's going to be used at build time, this can be changed by using the -mod flag of the go build command ( https://go.dev/ref/mod#build-commands )

The vendor dir has its use in disconnected build environments (RH's brew for example), and in case the repository for some of its dependencies would disappear, which sometimes happen, see https://github.com/openshift/oc/blob/9c5b183b3e692e58fd91b4b23bb7314819822865/go.mod#L188
github has a feature to automatically file PRs to update go.mod dependencies, see this PR for an example: containers/gvisor-tap-vsock#172

For crc-cloud, I would keep the vendor dir, CI safeguards can be added to make sure it has the expected content.

@praveenkumar
Copy link
Member

We have pushed those changes to pulumi branch and start consuming it. Closing this draft PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using pulumi as part of infrastructure provision
4 participants