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

Use up in exec if in PATH #495

Merged
merged 1 commit into from
May 7, 2024

Conversation

RedbackThomson
Copy link
Member

@RedbackThomson RedbackThomson commented May 7, 2024

Description of your changes

Fixes #490

The current logic for setting path to up in the Kubeconfig AuthInfo exec switches depending on the target release version. If we are on the release build, then use "up". If we are on the debug build, then use the current executable path. This has unfortunate side effects when customers use up but don't have the CLI in PATH - the AuthInfo won't ever be able to resolve the exec.

This change standardises the mechanism regardless of release target. The AuthInfo will always fall back to the full path of the up executable, unless we can guarantee we're running it from PATH.

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Running from PATH:

$ up ctx acmeco/upbound-gcp-us-west-1/default --profile production -f -
...
users:
- name: upbound
  user:
    exec:
      apiVersion: client.authentication.k8s.io/v1
      args:
      - organization
      - token
      command: up
...

Running from pwd:

$ ./up ctx upbound/upbound-gcp-us-west-1/nicks-group -f -
...
users:
- name: upbound
  user:
    exec:
      apiVersion: client.authentication.k8s.io/v1
      args:
      - organization
      - token
      command: /Users/nicholasthomson/Workspace/up/_output/bin/darwin_arm64/up
...

Copy link
Contributor

@cbuto cbuto left a comment

Choose a reason for hiding this comment

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

this worked for me!

small, non-blocking comment


// if the current executable was the same `up` that is found in PATH
path, err := exec.LookPath("up")
if err == nil && path == cmd {
cmd = "up"
Copy link
Contributor

Choose a reason for hiding this comment

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

what if path != cmd? do we need this check or should we always use whats returned by os.Executable()?

Copy link
Member Author

Choose a reason for hiding this comment

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

os.Exectuable will always return the full absolute path to up. I think it's cleaner for it to just use up and rely on the PATH instead of hard coding it. That's how other CLIs do it, and I feel it's a good model.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me

@RedbackThomson RedbackThomson merged commit 2380786 into upbound:main May 7, 2024
7 checks passed
@RedbackThomson RedbackThomson deleted the up-path-warning branch May 7, 2024 19:22
RedbackThomson added a commit to RedbackThomson/up that referenced this pull request May 7, 2024
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.

No warning when up isn't in the PATH
2 participants