[ENG-121] rpk: add `rpk connect` as a managed plugin #23117

r-vasquez · 2024-08-29T00:10:05Z

This PR introduces the required changes to have rpk connect as a managed plugin. The main behavior change is that from now on, rpk will try to download connect when running for the first time, similar to what we do with rpk cloud byoc.

It also introduces 3 new commands to rpk connect: install, uninstall, upgrade.

Fixes https://github.com/redpanda-data/devex/issues/45

Usage

Most common use case:

$ rpk connect run /config.yaml

^ That's it, it will download the connect plugin on the fly and then execute the command.
rpk won't update connect automatically unless you run rpk connect upgrade.

Manual Process

Install

# Install latest
$ rpk connect install

# Install specific version
$ rpk connect install --conect-version

# Install specific version if you already have connect installed
$ rpk connect uninstall
$ rpk connect install --connect-version  <version>
# or
$ rpk connect install --connect version <version> --force

Upgrade:

$ rpk connect upgrade

rpk only upgrades to latest, for specific versions use rpk connect install.

Uninstall:

$ rpk connect uninstall

Backports Required

Release Notes

Improvements

rpk connect will be downloaded on the first run.
rpk: introduce rpk connect install, rpk connect upgrade, and rpk connect uninstall, manual ways to manage rpk connect versioning.

r-vasquez · 2024-08-29T01:27:45Z

/dt

src/go/rpk/pkg/cli/connect/client.go

twmb · 2024-08-30T23:02:32Z

src/go/rpk/pkg/cli/connect/connect.go

+			out.MaybeDie(err, "unable to parse flags: %v", err)
+			connect, pluginExists := plugin.ListPlugins(fs, plugin.UserPaths()).Find("connect")
+			var pluginPath string
+			if !pluginExists {
+				path, err := installConnect(cmd.Context(), fs, "latest")
+				out.MaybeDie(err, "unable to install Redpanda Connect: %v", err)
+				pluginPath = path
+			}


Don't we only want to install the plugin if a person uses rpk connect <something else>? As in, don't force install a plugin if a person just does rpk connect -h?

Also, if we do install, here we probably want to print some message like, "connect plugin not found, downloading" etcetc

Connect can be run without any subcommand.

If you run rpk connect it starts a service, that's the main difference with byoc and that's why I chose to install it even for a --help

I think @Jeffail added rpk connect run. Can we block rpk connect -c / hijack it to not start the service, and rely entirely on subcommands?

Changed how it works, now:

When running without connect being installed:

$ rpk connect <Help text of the rpk bits: install, uninstall and upgrade> $ rpk connect --help <same as above> $ rpk connect --version cannot get connect version: rpk connect is not installed; run 'rpk connect install' <Help text> $ rpk connect -c foo... -c flag is not supported by this command; run 'rpk connect run' instead

If connect is installed, we will pass the flags to redpanda connect and they will work properly

src/go/rpk/pkg/cli/connect/uninstall.go

twmb · 2024-08-30T23:09:04Z

src/go/rpk/pkg/cobraext/cobraext.go

@@ -106,6 +106,9 @@ func StripFlags(args []string, fs *pflag.FlagSet, long []string, short []string)
 			if f == nil || !strip {
 				inFlag = len(kv) == 1
 				keep = append(keep, arg) // either we are keeping this flag, or it is not in our flag set so we cannot strip it
+				if i+1 < len(args) && strings.HasPrefix(args[i+1], "-") {
+					inFlag = false


Why? I think this is trying to detect the case of a bool flag, --verbose --anotherthing=whatever.

However if the first flag is --foo -bar, can't -bar be the value for --foo?

I think the logic you want here is IF the flag exists, then check if the flag is a bool flag. If so, THEN we can do this logic.

Why? I think this is trying to detect the case of a bool flag, --verbose --anotherthing=whatever.

Yes, that's correct. I've added a new comment.

However if the first flag is --foo -bar, can't -bar be the value for --foo?

Yes, it can be.

I think the logic you want here is IF the flag exists, then check if the flag is a bool flag. If so, THEN we can do this logic.

No, it can't be.

At this point, we know that the flag does not exist in rpk, we check this a couple of lines above. We cannot check if the flag is boolean because it's an unknown flag to rpk.

This flag is not used at the moment but it was missing the shorthand parsing.

This commit fixes a bug where if we were parsing a boolean long flag (let's say: --version), we will end up keeping the next argument after the flag, even if it was not needed.

We supported JSON and XML, now we add YAML.

vbotbuildovich · 2024-09-07T02:00:44Z

new failures in https://buildkite.com/redpanda/redpanda/builds/54125#0191c9eb-0513-415b-8ea6-1ece0e09bea1:

"rptest.tests.rpk_connect_test.RpkConnectTest.test_upgrade"

new failures in https://buildkite.com/redpanda/redpanda/builds/54125#0191ca00-e812-425c-9639-0a07b0639654:

"rptest.tests.rpk_connect_test.RpkConnectTest.test_upgrade"

src/go/rpk/pkg/cli/connect/connect.go

src/go/rpk/pkg/cli/connect/install.go

This change means that from now on, connect will be managed by rpk, so, if a user tries to run `rpk connect <...>` for the first time, rpk will download, install and execute the plugin (connect). It also introduces 3 new commands to `rpk conenct` that live in rpk: install, uninstall, and upgrade.

github-actions bot added the area/rpk label Aug 29, 2024

twmb reviewed Aug 30, 2024

View reviewed changes

src/go/rpk/pkg/cli/connect/client.go Outdated Show resolved Hide resolved

twmb previously approved these changes Aug 30, 2024

View reviewed changes

r-vasquez added 3 commits September 6, 2024 14:26

rpk: add LongFlagValue support for shorthand flags

3c75192

This flag is not used at the moment but it was missing the shorthand parsing.

rpk: fix plugin flag parsing of boolean flags

e2dff07

This commit fixes a bug where if we were parsing a boolean long flag (let's say: --version), we will end up keeping the next argument after the flag, even if it was not needed.

rpk/httpapi: add yaml as fallback during unmarshal

1a49b90

We supported JSON and XML, now we add YAML.

r-vasquez dismissed twmb’s stale review via 5754156 September 6, 2024 23:04

r-vasquez force-pushed the rpk-connect-1 branch 2 times, most recently from 5754156 to 5dbd5c1 Compare September 6, 2024 23:12

r-vasquez marked this pull request as ready for review September 6, 2024 23:20

r-vasquez requested review from gene-redpanda and Deflaimun as code owners September 6, 2024 23:20

r-vasquez requested a review from jackietung-redpanda September 6, 2024 23:32

r-vasquez force-pushed the rpk-connect-1 branch from 5dbd5c1 to c81147d Compare September 9, 2024 16:37

r-vasquez requested a review from twmb September 10, 2024 16:21

gene-redpanda reviewed Sep 10, 2024

View reviewed changes

src/go/rpk/pkg/cli/connect/connect.go Outdated Show resolved Hide resolved

gene-redpanda reviewed Sep 10, 2024

View reviewed changes

src/go/rpk/pkg/cli/connect/connect.go Outdated Show resolved Hide resolved

gene-redpanda reviewed Sep 10, 2024

View reviewed changes

src/go/rpk/pkg/cli/connect/install.go Show resolved Hide resolved

gene-redpanda reviewed Sep 10, 2024

View reviewed changes

src/go/rpk/pkg/cli/connect/install.go Show resolved Hide resolved

r-vasquez force-pushed the rpk-connect-1 branch from c81147d to 07f774d Compare September 10, 2024 23:25

r-vasquez force-pushed the rpk-connect-1 branch from 07f774d to b3e9211 Compare September 10, 2024 23:30

twmb approved these changes Sep 11, 2024

View reviewed changes

r-vasquez merged commit 35bbc27 into redpanda-data:dev Sep 11, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-121] rpk: add `rpk connect` as a managed plugin #23117

[ENG-121] rpk: add `rpk connect` as a managed plugin #23117

r-vasquez commented Aug 29, 2024 •

edited

Loading

r-vasquez commented Aug 29, 2024

twmb Aug 30, 2024

r-vasquez Aug 30, 2024

twmb Sep 3, 2024

r-vasquez Sep 6, 2024 •

edited

Loading

twmb Aug 30, 2024

r-vasquez Sep 7, 2024 •

edited

Loading

vbotbuildovich commented Sep 7, 2024 •

edited

Loading

[ENG-121] rpk: add rpk connect as a managed plugin #23117

[ENG-121] rpk: add rpk connect as a managed plugin #23117

Conversation

r-vasquez commented Aug 29, 2024 • edited Loading

Usage

Most common use case:

Manual Process

Install

Upgrade:

Uninstall:

Backports Required

Release Notes

Improvements

r-vasquez commented Aug 29, 2024

twmb Aug 30, 2024

Choose a reason for hiding this comment

r-vasquez Aug 30, 2024

Choose a reason for hiding this comment

twmb Sep 3, 2024

Choose a reason for hiding this comment

r-vasquez Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

When running without connect being installed:

twmb Aug 30, 2024

Choose a reason for hiding this comment

r-vasquez Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

vbotbuildovich commented Sep 7, 2024 • edited Loading

[ENG-121] rpk: add `rpk connect` as a managed plugin #23117

[ENG-121] rpk: add `rpk connect` as a managed plugin #23117

r-vasquez commented Aug 29, 2024 •

edited

Loading

r-vasquez Sep 6, 2024 •

edited

Loading

When running without `connect` being installed:

r-vasquez Sep 7, 2024 •

edited

Loading

vbotbuildovich commented Sep 7, 2024 •

edited

Loading