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

Fix and document go install method of installation #718

Open
marshall007 opened this issue Jun 21, 2024 · 11 comments
Open

Fix and document go install method of installation #718

marshall007 opened this issue Jun 21, 2024 · 11 comments
Assignees
Labels
possible-bug Something may not be working

Comments

@marshall007
Copy link

Environment

Device and OS: Linux
App version: >= v0.11.1

Steps to reproduce

  1. go install github.com/defenseunicorns/uds-cli@v0.11.0 works
  2. go install github.com/defenseunicorns/uds-cli@latest does not

Additionally, it would be great if we created a module named cmd/uds so that go install github.com/defenseunicorns/uds-cli/cmd/uds@latest creates a binary named uds instead of uds-cli. Alternatively, we could detect of the binary name when calling the CLI program so that calling ./uds in tasks is rewritten to the correct path place no matter what the host binary is named.

Visual Proof (screenshots, videos, text, etc)

$ go install github.com/defenseunicorns/uds-cli@latest
go: downloading github.com/defenseunicorns/uds-cli v0.11.2
go: github.com/defenseunicorns/uds-cli@v0.11.2 requires go >= 1.22.3; switching to go1.22.4
go: downloading go1.22.4 (linux/amd64)
go: github.com/defenseunicorns/uds-cli@latest (in github.com/defenseunicorns/uds-cli@v0.11.2):
        The go.mod file for the module providing named packages contains one or
        more replace directives. It must not contain directives that would cause
        it to be interpreted differently than if it were the main module.

Additional Context

The replace directives added in #662 (see go.mod#L530-L535) prevent this method of installation.

replace github.com/prometheus/common => github.com/prometheus/common v0.45.0

replace github.com/docker/docker => github.com/docker/docker v24.0.9+incompatible

// remove when Zarf updates k9s versions to v0.32.4
replace github.com/derailed/k9s => github.com/derailed/k9s v0.32.4
@marshall007
Copy link
Author

marshall007 commented Jun 21, 2024

Looks like we evaluated this in #583 (comment) and it can't be done because these build args are required?

BUILD_ARGS := -s -w -X 'github.com/defenseunicorns/uds-cli/src/config.CLIVersion=$(CLI_VERSION)' \
				    -X 'github.com/defenseunicorns/zarf/src/config.ActionsCommandZarfPrefix=zarf'

Is that all? Surely we could default github.com/defenseunicorns/zarf/src/config.ActionsCommandZarfPrefix="zarf" in the upstream package?

@marshall007
Copy link
Author

Does github.com/defenseunicorns/zarf/src/config.ActionsCommandZarfPrefix default to empty string because we want un-prefixed commands like ./tools to work instead of ./zarf tools when using the zarf CLI directly?

  1. I think that is ambiguous and we should require the ./zarf prefix in cmd scripts
  2. We could still support both prefixed and non-prefixed vendor commands
  3. This logic seems incorrect in any case. Shouldn't we check args[0] == config.ActionsCommandZarfPrefix before popping it off? https://github.com/defenseunicorns/zarf/blob/1b104dc75177d2fee37e539da092ca4400b4af15/src/cmd/common/vendor.go#L52-L54

@UncleGedd
Copy link
Collaborator

Looks like we evaluated this in #583 (comment) and it can't be done because these build args are required?

BUILD_ARGS := -s -w -X 'github.com/defenseunicorns/uds-cli/src/config.CLIVersion=$(CLI_VERSION)' \
				    -X 'github.com/defenseunicorns/zarf/src/config.ActionsCommandZarfPrefix=zarf'

Is that all? Surely we could default github.com/defenseunicorns/zarf/src/config.ActionsCommandZarfPrefix="zarf" in the upstream package?

That build arg forces bundled Zarf packages that use Zarf Actions to use the vendored zarf in the uds binary (as well as the tools Zarf itself vendors). Not sure how it works in Zarf but I don't think we could set that in the upstream (@AustinAbro321 correct me if I'm wrong!)

@UncleGedd
Copy link
Collaborator

Noting too that we use many build args (as does Zarf)

@UncleGedd
Copy link
Collaborator

UncleGedd commented Jun 21, 2024

Appreciate the issue @marshall007 ! Given the complexity of removing the build args, I'd like to better understand the use case for doing go install over the install instructions found in the UDS CLI README and official docs.

@UncleGedd
Copy link
Collaborator

UncleGedd commented Jun 21, 2024

^ also if I'm missing something and we can do go install without the build args I'm all ears! FWIW, I feel like either grabbing the binary directly from Github or installing via brew (as documented) is pretty normal

@UncleGedd
Copy link
Collaborator

creates a binary named uds instead of uds-cli

If installing from brew we intentionally name the binary uds

@marshall007
Copy link
Author

@UncleGedd I'm on Linux. I could use linuxbrew but its a bit clunky in my experience. This is totally a preference/convenience thing but go install just always works and automatically downloads and compiles with the correct version of go for the given package. I could go download from release artifacts, but that makes up/downgrades between minor versions (which I actively develop against) annoying.

Noting too that we use many build args (as does Zarf)

All of these build args you've linked to are seemingly only used for our own wrapper around zarf tool <tool> --version: https://github.com/defenseunicorns/zarf/blob/1b104dc75177d2fee37e539da092ca4400b4af15/src/cmd/tools/helm.go#L30-L32

I think we could get the same (or better) information using the runtime/debug.ReadBuildInfo() API. They do not need to be constants we set as build args because this metadata is already baked into the binaries.

@UncleGedd
Copy link
Collaborator

Great points. This issue will be tough for the team to prioritize in the coming weeks, but we'd happily welcome a PR if you're interested

@marshall007 marshall007 self-assigned this Jun 24, 2024
@marshall007
Copy link
Author

Thanks, I will assign myself and try to get around to it this week.

@marshall007
Copy link
Author

marshall007 commented Jun 26, 2024

Ok it's a bit of a mixed bag, but I think all the variables that are actually functional/load-bearing can be set at runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
possible-bug Something may not be working
Projects
None yet
Development

No branches or pull requests

2 participants