Add brew support for tel2oss version builds

telepresenceio / telepresence

Local development against a remote Kubernetes or OpenShift cluster

https://www.telepresence.io

Other

6.53k stars 513 forks source link

Add brew support for tel2oss version builds #3626

Closed tonur closed 2 months ago

tonur commented 3 months ago

Description

Added support for installing the Open Source version of Telepresence (tel2oss) via a Homebrew formula originally created here: https://github.com/tonur/homebrew-telepresence-oss. Closes #3609

Checklist

Regarding the checklist, I am not sure that this change constitutes an update to the CHANGELOG.yml or any release notes. I would be happy if the maintainers would comment on this to see if this is necessary.

[x] I made sure to update ./CHANGELOG.yml.
[x] I made sure to add any docs changes required for my change (including release notes).
[x] My change is adequately tested.
[x] I updated CONTRIBUTING.md with any special dev tricks I had to use to work on this code efficiently.
[x] I updated TELEMETRY.md if I added, changed, or removed a metric name.
[x] Once my PR is ready to have integration tests ran, I posted the PR in #telepresence-dev in the datawire-oss slack so that the "ok to test" label can be applied.

Waiting to check the last item until I get feedback on the first two items on the checklist.

thallgren commented 3 months ago

This looks good to me. And yes, I think it would be great if you could add an entry to the changelog.

tonur commented 3 months ago

After this PR, a change could then be made to telepresenceio/telepresence.io/docs/latest/quick-start/index.md, to be like the telepresenceio/telepresence.io/docs/pre-release/install/index.md:

# Install via brew:
brew install datawire/blackbird/telepresence-oss

tonur commented 3 months ago

I am so sorry about the amount of noise in this PR, I had some issues with syncing with the origin, due to me trying to do a git merge release/v2 && git commit --signoff, so I ended up doing a rebase instead. Apologies! I would appreciate a quick re-review to ensure that everything is still in order, to your standards.

thallgren commented 3 months ago

@tonur there's one more thing that needs to happen. The version of the tel2 and tel2oss are not aligned, so the makefile must have two explicit targets. We won't publish both at the same time.

tonur commented 3 months ago

I am looking into modifying the makefile to have two targets today, will request a review again when this is done. Thanks for letting me contribute!

tonur commented 3 months ago

I must admit, I am not totally clear how the "tel2" package is built. Is it built from this repository?

I assume that by introducing two targets, I have to modify the UserClient name to be "OSS Client" when building tel2oss and "Client" when building tel2, but I have difficulties finding out where to modify this name. It seems to come from the UserClient name found in the getConnectorVersion method here: https://github.com/telepresenceio/telepresence/blob/release/v2/pkg/client/cli/connect/connector.go#L336 Is this correct? Can you perhaps point me in the right direction on how to proceed @thallgren? Especially how to compile the non oss tel2 version, since I have issues building other versions than the tel2oss version by running make build that builds the "./build-output/bin/telepresence" file.

thallgren commented 3 months ago

The name you point to is the name of the connection. The name that the binary reports as "OSS Client" is set here:

https://github.com/telepresenceio/telepresence/blob/release/v2/pkg/client/cli/main.go#L45

and it is set differently when building the enterprise version of Telepresence. I don't think it's relevant to this PR at all.

That said, and sorry for the confusion earlier, I think you need one target for the tel2oss only. The tel2 package is currently built by calling the from the enterprise Makefile like so:

    packaging/homebrew-package.sh $(patsubst v%,%,$(TELEPRESENCE_VERSION)) $(GOARCH)

and we'll need to change the last argument there to tel2 instead of $(GOARCH) while you keep the tel2oss here.

tonur commented 2 months ago

Hey again @thallgren. Apologies for the delay, I have been occupied with a job change. I removed the tel2 target, so I think everything is good on my end if this PR only requires a change in the enterprise Makefile (Which I don't know where where it is located. I assume it is elsewhere, non public). Otherwise, if I need to make other changes, please let me know.

thallgren commented 2 months ago

Hey @P0lip , Just a heads up. I intend to merge this shortly, and it will require a small change in the enterprise code when you decide to include it. See this comment.