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

Move ctp connect to ctp connector install #389

Merged
merged 2 commits into from
Oct 8, 2023

Conversation

tnthornton
Copy link
Member

Description of your changes

This PR shuffles the subcommands around a little. This is in anticipation of some reworking for ctp connect.

This changeset builds on #388. #388 should be merged/reviewed prior to this PR.

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

./_output/bin/darwin_arm64/up ctp --help
Usage: up controlplane (ctp) <command>

Interact with control planes.

Flags:
  -h, --help                         Show context-sensitive help.
      --format="default"             Format for get/list commands. Can be: json, yaml, default
  -v, --version                      Print version and exit.
  -q, --quiet                        Suppress all output.
      --pretty                       Pretty print output.

      --domain=https://upbound.io    Root Upbound domain ($UP_DOMAIN).
      --profile=STRING               Profile used to execute command ($UP_PROFILE).
  -a, --account=STRING               Account used to execute command ($UP_ACCOUNT).
      --insecure-skip-tls-verify     [INSECURE] Skip verifying TLS certificates
                                     ($UP_INSECURE_SKIP_TLS_VERIFY).
  -d, --debug=INT                    [INSECURE] Run with debug logging. Repeat to increase verbosity.
                                     Output might contain confidential data like tokens ($UP_DEBUG).

Commands:
  controlplane (ctp) create           Create a managed control plane.
  controlplane (ctp) delete           Delete a control plane.
  controlplane (ctp) list             List control planes for the account.
  controlplane (ctp) get              Get a single control plane.
  controlplane (ctp) connector        Connect an App Cluster to a managed control plane.
  controlplane (ctp) configuration    Manage Configurations.
  controlplane (ctp) provider         Manage Providers.
  controlplane (ctp) pull-secret      Manage package pull secrets.
  controlplane (ctp) kubeconfig       Manage control plane kubeconfig data.
./_output/bin/darwin_arm64/up ctp connector --help
Usage: up controlplane (ctp) connector <command>

Connect an App Cluster to a managed control plane.

Flags:
  -h, --help                         Show context-sensitive help.
      --format="default"             Format for get/list commands. Can be: json, yaml, default
  -v, --version                      Print version and exit.
  -q, --quiet                        Suppress all output.
      --pretty                       Pretty print output.

      --domain=https://upbound.io    Root Upbound domain ($UP_DOMAIN).
      --profile=STRING               Profile used to execute command ($UP_PROFILE).
  -a, --account=STRING               Account used to execute command ($UP_ACCOUNT).
      --insecure-skip-tls-verify     [INSECURE] Skip verifying TLS certificates
                                     ($UP_INSECURE_SKIP_TLS_VERIFY).
  -d, --debug=INT                    [INSECURE] Run with debug logging. Repeat to increase verbosity.
                                     Output might contain confidential data like tokens ($UP_DEBUG).

Commands:
  controlplane (ctp) connector install    Install mcp-connector into an App Cluster.

Copy link
Contributor

@AlainRoy AlainRoy left a comment

Choose a reason for hiding this comment

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

This is way better than I would have done, had I finished it. It looks great, and I like the approach to having a unified response for the two kinds of control planes.

My only comments are nits -- very small.

One question: The description for the PR sounds much more limited than what is actually here. Is the description wrong, or did you not intend to include everything in this PR? I'm okay with it all being included, just making sure it's what you expect.

Other than that last concerns (about the description matching the contents), I'm happy and will pre-emptively approve, assuming you'll fix the description if appropriate.

cmd/up/controlplane/connector/connector.go Outdated Show resolved Hide resolved
internal/controlplane/cloud/cloud.go Outdated Show resolved Hide resolved
@AlainRoy
Copy link
Contributor

AlainRoy commented Oct 8, 2023

Wait, I looked that the PRs in the wrong order: this overlaps #388. I'm guessing this PR needs to be updated? Maybe you should ignore my approval there.

@tnthornton
Copy link
Member Author

Wait, I looked that the PRs in the wrong order: this overlaps #388. I'm guessing this PR needs to be updated? Maybe you should ignore my approval there.

Ya, this is branched from #388 and builds on that guy. I called that out in the description too 👍 . I'm guessing this is the reason for this comment:

One question: The description for the PR sounds much more limited than what is actually here. Is the description wrong, or did you not intend to include everything in this PR? I'm okay with it all being included, just making sure it's what you expect.

?

@AlainRoy
Copy link
Contributor

AlainRoy commented Oct 8, 2023

Wait, I looked that the PRs in the wrong order: this overlaps #388. I'm guessing this PR needs to be updated? Maybe you should ignore my approval there.

Ya, this is branched from #388 and builds on that guy. I called that out in the description too 👍 . I'm guessing this is the reason for this comment:

One question: The description for the PR sounds much more limited than what is actually here. Is the description wrong, or did you not intend to include everything in this PR? I'm okay with it all being included, just making sure it's what you expect.

?

Yeah, I just got confused since it's awaiting the merge, so there was more code. It all looks good though.

Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
@tnthornton tnthornton merged commit 353ea63 into upbound:main Oct 8, 2023
6 checks passed
@tnthornton tnthornton deleted the move-mcp-connect branch October 8, 2023 23:16
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

2 participants