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

[email protected] 1.17.3 (new formula) #79695

Closed

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Jun 21, 2021

This adds [email protected] and backfills an alias for 1.18, as that's the current version.

Notably, Envoy 1.18 changed the config format in an incompatible way with 1.17 which is supported until next year

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

cd Aliases;ln -s ../Formula/envoy.rb '[email protected]'

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Jun 21, 2021
@carlocab
Copy link
Member

Why do we need this alias? The alias will need to change when Envoy is upgraded to 1.19, so anyone using brew install [email protected] will just get an error when that happens.

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Jun 22, 2021

good q. minors are usually not compatible in envoy though what isn't compatible may not effect everyone.

For example, the pending incompatibilities of 1.19 aren't as shocking as 1.18 which dropped a config format

Knowing this, I'll leave it up to your judgement as I agree alias maintenance is added!

@carlocab
Copy link
Member

It's not really a question of alias maintenance. I just don't see how this alias will be useful. Or do you think brew install [email protected] returning an error when Envoy is upgraded to 1.19 would be useful?

@codefromthecrypt
Copy link
Contributor Author

TL;DR; I think most won't need this alias, and only some developers would want to pin to a minor. The problem is there is no packaging for OS/x except homebrew at the moment.

A developer of wasm (filter plugins) may want to know the version is what they think it is and won't care much about the patch version (though sometimes they also break). This has to do with the features changing fast in envoy and that there are a few non-EOL versions https://github.com/envoyproxy/envoy/blob/main/RELEASES.md#release-schedule

I see what you are saying that basically no one can rely on [email protected] for very long so possibly it is not worth doing.. that they should either not use homebrew to get a known minor, or go straight to the ghcr.io registry?

If there's a way to retain the existing last [email protected] without copy/pasting the formula, that would be best, but I don't know that option and only know the alias thing or copy/paste. If there's another way all 👂 s

@carlocab
Copy link
Member

When Envoy 1.19 is released, will Envoy 1.18 continue to receive updates? If it will, and it also meets the rest of the criteria at https://docs.brew.sh/Versions#acceptable-versioned-formulae, then it may be possible to add an [email protected] formula when 1.19 is released.

@codefromthecrypt
Copy link
Contributor Author

early July 1.19 will be available and a few others including 1.18 will be non-EOL, which means patch releases. So probably the worst case is for a week there could be 4 non-EOL versions out there, but since we are starting with 1.18 and usually one ages out, typically it would be 3 current non-EOL versions and of course, here we could decide to do less.

@codefromthecrypt
Copy link
Contributor Author

probably the bigger point on https://docs.brew.sh/Versions#acceptable-versioned-formulae is "large number of people" frankly I don't think that's the case now, especially vs something like ruby.

I suppose some stats could show that's the case or not, but I'm guessing this might be a reason to close this until it becomes more popular...

@carlocab
Copy link
Member

A "large number of people" isn't that demanding a requirement. At the moment it amounts to ~800 installs a year. If it seems likely to cross that, then we could probably have an [email protected] formula. All the earlier versions probably won't qualify, though.

That said, it's still not clear adding this alias now is still useful. Even if we merge this, and I do brew install [email protected] now, I will still get Envoy 1.19 when I do brew upgrade (when Envoy 1.19 is merged here).

@codefromthecrypt
Copy link
Contributor Author

I think the primary use case for brew install [email protected] is CI matrix or ad-hoc switching back to the prior version because something busted or you were supporting something and there was some doubt about the version being implicated.

@carlocab
Copy link
Member

Yes. And that'll be possible if/when there is an [email protected] formula. I don't think it's a good idea to have users relying on its existence until the PR that adds one is accepted.

@codefromthecrypt
Copy link
Contributor Author

gotcha. so basically this is the wrong solution to the problem. Do you prefer a different PR to change the implementation or keep this one? I can see value in the comments here so inclined to change the commits instead of open+ref, but up to you

@SMillerDev
Copy link
Member

You can just update this one

@codefromthecrypt codefromthecrypt changed the title envoy: add alias [email protected] envoy: adds [email protected] (last envoy version to support config v2) Jun 30, 2021
@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Jun 30, 2021

OK I updated this to backfill only one previous version 1.17. This is important because its config version is still supported until next year. Hope this is ok, but please tell me if there are any issues with it*

  • I noticed many formulas alias [email protected] -> ../Formula/x.rb until x.rb becomes a historical version. It seems sensible to do this also here as it will follow as a versioned formula when 1.19 is out anyway.

@carlocab
Copy link
Member

Please squash your commits together and use the commit message [email protected] 1.17.3 (new formula)

@carlocab carlocab changed the title envoy: adds [email protected] (last envoy version to support config v2) [email protected] 1.17.3 (new formula) Jun 30, 2021
@codefromthecrypt
Copy link
Contributor Author

thanks so much for your help @carlocab I will try to pay back by maintaining this

@codefromthecrypt
Copy link
Contributor Author

fyi envoy doesn't work with macOS+arm64 yet due to things including envoyproxy/envoy#16083 (comment)

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @codefromthecrypt. Away from my computer at the moment, so can't trigger a merge. Ping me later this evening (GMT) if I forget.

@codefromthecrypt
Copy link
Contributor Author

cool looking forward to it!

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@codefromthecrypt codefromthecrypt deleted the envoy-alias branch July 4, 2021 01:24
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants