Skip to content

💣 ditch vendor/ #7460

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

srenatus
Copy link
Contributor

No description provided.

@srenatus srenatus force-pushed the push-wqmszzonoupk branch from f4b9a8b to 436212e Compare March 21, 2025 12:23
Copy link

netlify bot commented Mar 21, 2025

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit 3a83877
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/67fcd46eafb7db0008536e34
😎 Deploy Preview https://deploy-preview-7460--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@philipaconrad
Copy link
Contributor

Oh wow! This is a delightful change, and should make OPA development much more straightforward for newcomers. 🙂

The friction from the go mod vendor steps was small, but significant.

@tjons
Copy link
Contributor

tjons commented Apr 6, 2025

echo @philipaconrad... this is delightful! I would like to get this into opa-envoy-plugin as well as the vendor directory there is getting large and unwieldy... anything I can do to help get this over the line here @srenatus? I'll wait to make that change until we see success here

@srenatus
Copy link
Contributor Author

srenatus commented Apr 6, 2025

@tjons Sorry for not providing context earlier: I've been trying to update our immensely outdated wasmtime-go version (#7441), but due to the way that wasmtime-go works, it'll add ~350M of binary blobs to vendor/ and hence our git history, for every version we incorporate.

Now, there are some approaches to this problem:

  1. Verify that it's not a problem at all because of shallow clones etc.
  2. Ditch vendor/ from git tracking
  3. Ditch wasmtime-go in OPA.
    1. Remove the ability to evaluate OPA's wasm modules in OPA, or
    2. Replace wasmtime-go with wazero.

This PR was to experiment with (2.). When pushed, it worked fine without too many changes.

For (3.ii), I had been playing with this before, but I ran into some sort of road block that I only vaguely remember. I think it had to do with wazero not supporting something weird we did in our exported wasm modules (importing and exporting memory?). So, this is something to verify. ❗As a side note, if we cannot evaluate our wasm modules with wazero, noone else can, and we should fix that.

As for (2.), I'm open to removing the git tracking of vendor/, but we've been having internal discussions about that a few times, and never reached a clear consensus. I take it you'd be in favor of doing so, and don't see any downsides related to not having vendor/ under git tracking?

@johanfylling
Copy link
Contributor

Possible pain-points from dropping vendoring:

  1. Some part in go dependency tooling goes belly up, and we lose the ability to patch an older release
    1. High hazard, low risk. For a dep to get lost, its repo would need to have been dropped, and its entry scrubbed from the default go module proxy's cache. If we in the future feel the default module proxy is unreliable, we can reintroduce vendoring or select/host a different proxy.
  2. No hermetic builds.
    1. Kinda solved by go.mod/go.sum.
  3. Builds may take longer as deps might need to be downloaded if not locally cached.
    1. So we wait a bit more.
  4. Switching between branches may take longer, depending on dep differences
    1. Unlikely to be an issue, as dependabot has a monthly cadence, and we don't expect a lot of differences between branches (unless for long-lived branches, but they should be synced with main regularly anyways.
  5. Offline work (e.g. while traveling) can result in local branches that can't be built because of missing deps.

Are there others?

Generally, the risk and pain involved in dropping vendor/ seem pretty low. IMO, this experiment is a preferred option to dropping Wasm runtime support. Also, we can reverse this if we encounter issues. We might lose the ability to patch ancient OPA versions, but that's something we avoid doing anyways.

@srenatus
Copy link
Contributor Author

I think it's pretty accurate, thanks for summing it up! I can revive this PR on Monday, but I wouldn't mind if someone beat me to it.

@srenatus srenatus force-pushed the push-wqmszzonoupk branch 2 times, most recently from c48229a to d0be0c7 Compare April 14, 2025 09:20
@srenatus srenatus force-pushed the push-wqmszzonoupk branch from d0be0c7 to 3a83877 Compare April 14, 2025 09:24
@srenatus srenatus marked this pull request as ready for review April 14, 2025 13:55
@srenatus
Copy link
Contributor Author

We might lose the ability to patch ancient OPA versions, but that's something we avoid doing anyways.

What is this idea based on, btw? The go module proxy forgetting about old version? Trying to figure out if this was a possibility, I ran into this comment

We are not going to delete formerly published versions from the proxy storage, nor from the @v/list, which is how programs access that storage.

So, I don't think this is really an issue.

@srenatus srenatus changed the title 🚧 💣 ditch vendor/ 💣 ditch vendor/ Apr 14, 2025
@johanfylling
Copy link
Contributor

@srenatus, it's a worst-case scenario. Policies change, commitments change, etc.

Copy link
Contributor

@tjons tjons left a comment

Choose a reason for hiding this comment

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

Not a maintainer, but adding my approval as this is a nice quality-of-life improvement

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.

None yet

4 participants