(feat) Plugin version constraints

[moensch]

Implements #1335

This add support for a ?constraint query parameter on the plugin definition. It triggers the bootstrapper to list all semver-compatible tags on the plugin repository and finds the highest version match using https://github.com/Masterminds/semver.

Example:

steps:
  - command: test.sh
    plugins:
      - docker-compose?constraint=^3.x:
          run: app

Some notes:

• It would have been more "streamlined" to execute the git ls-remote from within the plugin package, but I didn't want to introduce shell to it - hence it's kept within bootstrap
• Because of ^^, .Version is now lazily set. If the ls-remote were done within the plugin package, we could set this early (in CreatePlugin())
• Intentionally no fails/errors on any git tags which do not match semver
• This implementation would require changes to the server side pipeline parser since repo url query parts are discarded. It could alternatively be implemented by prefixing the url fragment like org/repo#constraint:%3C%3D1.0 (url encoded less-than, pipeline parser misbehaved when I passed in org/repo#constraint:<1.0

[chloeruka]

This is awesome, thanks for the PR! We'll give this some time for testing ASAP.

[lox]

There is a lot of historical context here (which perhaps we should ignore), but I'll link it here anyway:

https://github.com/buildkite/agent/pull/1025 https://github.com/buildkite/agent/pull/1034

[moensch]

This is awesome, thanks for the PR! We'll give this some time for testing ASAP.

Thank you - I did notice that the pipeline parser API is discarding url query parts - so I tested this through unit/integration tests. I have a local version that's piggy-backing on the url fragment, but didn't want to update the PR here (I dislike having one thing have multiple meanings based on just some regex rules).

I did have to URL encode < and = in my YAML for it to work, though I believe this could be overcome in other parts of the agent or the pipeline api.

[pda]

Hey @moensch, thanks for the pull request.

I noticed this in your issue:

My proposal would check for a new version on each run still since I use the "resolved" actual version as the .Identifier() and not the constraint. — https://github.com/buildkite/agent/issues/1335#issuecomment-725713670

I had some concerns about that in the earlier thread:

The other concern is rate limiting — if a plugin is used org-wide in a large organisation, it could be triggered thousands of times per minute. Apart from being inefficient, fetching it each time could incur rate limiting from the plugin repo provider (👋🏼 GitHub). — https://github.com/buildkite/agent/issues/422#issuecomment-675163573

I think I'd still be more comfortable caching the matched version for up to an hour (or some other reasonable amount of time). That would mean a time window after a new plugin release where some agents still have the old matching version and some have the newer matching version, but maybe that's no big deal… that's kind of part of the semver contract.

---

Also, I'm wondering what led you from docker-compose#^3.x in https://github.com/buildkite/agent/issues/1335 to docker-compose?constraint=^3.x in this pull request? Aesthetically I like the former. And it seems you've run into server-side issues with the ? approach. Based on the comments in @toolmantim's https://github.com/buildkite/agent/pull/1025 and your own comments here it looks like sticking with a hash syntax will avoid those issues. Was there a backwards compatibility concern? Or future-proofing for other types of version selector?

[moensch]

Thanks @pda for taking time to review this!

Also, I'm wondering what led you from docker-compose#^3.x in #1335 to docker-compose?constraint=^3.x

I was pondering using the URL fragment, but I am personally not a fan of overloading the same "data element" for multiple purposes. How would you differentiate between the URL fragment being a git commit-ish or a version constraint? I foresaw some less-than-pretty regexp to make that differentiation, hence I opted for a query arg instead.

I played with this further locally (since I couldn't run an end-to-end test with query args) and have a version where I am using the URL fragment, but in the format of:

buildkite-plugins/docker-compose#constraint:<1.2.0

That way I could just do a strings.HasPrefix() which seemed cleaner.

caching the matched version for up to an hour

I am a +1 on caching, I just did not yet want to invest the extra time before I had some reassurance that the general direction of this PR is actually acceptable. If it is, then I'm happy to add caching.

[staticfloat]

For the record, we're simulating this by using a github action to automatically set (and update) a v1 tag on our repositories so that we can declare e.g. julia#v1 and get the latest matching v1.X version. That has obvious downsides (including the need for an agent environment hook to update the git tags) so we'd love to see this pushed through for proper semver support!

[moensch]

I would love to start up the conversation on this PR again around proper semver support for buildkite plugins. I am happy to keep iterating on this PR, but I suspect that without some support in the back end, what I can come up with will feel like a rather dirty workaround.

[moskyb]

kia ora @moensch! just wanted to update you on what's going on with this internally, as we've been a bit quiet.

The good news is that this PR could basically be shipped as-is - it's awesome work, and is really well written. There'd need to be a couple of backend changes on our end, but it's on the scale of hours of work - I could do them pretty quickly, and I'm not a rails person (anymore).

The less good news is that there's a bunch of complexity surrounding the way that plugins get pulled in that we need to sort out - at the moment, plugins basically get pulled once, the first time that the agent needs to run them, and notices that they haven't been pulled yet.

This leads to a bunch of ✨interesting✨ (read: annoying) edge cases with regards to this PR specifically. For example, let's say that we've shipped this PR and its surrounding backend changes and everything works as intended. If you updated your agent to use this feature, and then used the pipeline yaml fragment in the PR description, if your agent already had a copy of the docker-compose plugin, it would silently just use that instead, and disregard the version constraint you've specified.

It's worth noting that this is already an issue in the agent - if you specify #main as the commit to use for a plugin, the agent will install some-plugin#main at that point in time, and the plugin version will get glued to whatever commit main points to at that point in time until the plugin directory gets cleaned out at some point.

IMO one of the solutions on offer here is to implement some sort of plugin cachebusting system - eg, every hour (or whatever, the interval isn't that important), go through all the plugins the agent has checked out, and make sure that whatever version we've asked for is still the most recent possible (eg, plugin#main => "has the main commit changed? if so, pull the latest main and use that instead", plugin?constraint=^3.x => "we previously resolved this constraint to 3.3, but 3.4 is out, so let's pull that", etc). There's quite a lot of complexity hiding in this process, I think.

All this is to say: prior to merging this, we need to implement something along the lines of this cachebuster, otherwise we risk some really confusing behaviour from this PR. This sucks, and i'm really sorry about it. We're going to be looking into implementing some sort of cachebusting behaviour, but unfortunately i can't give any estimates as to when we'll get to it - the agent dance card is rather full at the moment.