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

Kusk CLI version - Partially closes kubeshop/kusk-gateway#415 #37

Merged
merged 5 commits into from
May 23, 2022

Conversation

mbana
Copy link
Contributor

@mbana mbana commented May 18, 2022

Kusk CLI version - Partially closes kubeshop/kusk-gateway#415

Define a version command that uses value of github.com/kubeshop/kusk-gateway/pkg/build populated by ldflags:

  • github.com/kubeshop/kusk-gateway: Get latest version that includes github.com/kubeshop/kusk-gateway/pkg/build package.
  • github.com/kubeshop/kusk-gateway/pkg/build: Version variable will contain the latest tag when built.
  • TELEMETRY_TOKEN: Remove references to ldflags/build-arg that is not used.
  • .github/workflows/go.yml: Remove test-GoReleaser-build as we can just run it locally using make build-goreleaser.
  • .goreleaser.yml: Populate KuskGAMeasurementID based on secret/env.
  • .goreleaser.yml: Populate KuskGAApiSecret based on secret/env.

Example run log

$ make build
Makefile:62: warning: undefined variable 'GA_ID'
Makefile:62: warning: undefined variable 'GA_SECRET'
go build -v -o ./kusk -ldflags="-X 'github.com/kubeshop/kusk-gateway/pkg/build.Version=v1.0.4-5-ga63345e' -X 'github.com/kubeshop/kusk-gateway/pkg/analytics.KuskGAMeasurementID=' -X 'github.com/kubeshop/kusk-gateway/pkg/analytics.KuskGAApiSecret='" ./main.go
$ ./kusk version
kusk version 1.0.4-5-ga63345e
https://github.com/kubeshop/kusk/releases/latest
$ ./kusk --version
kusk version 1.0.4-5-ga63345e
https://github.com/kubeshop/kusk/releases/latest
$ ./kusk -v
kusk version 1.0.4-5-ga63345e
https://github.com/kubeshop/kusk/releases/latest

This PR...

Changes

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@mbana mbana changed the title Fix/Cleanup Makefile - Partially closes kubeshop/kusk-gateway#418: Kusk CLI version - Partially closes kubeshop/kusk-gateway#418 May 18, 2022
@mbana mbana self-assigned this May 18, 2022
@mbana mbana marked this pull request as ready for review May 18, 2022 17:20
Copy link
Contributor

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

I recommend we add version at build time just like I did in kusk-gayeway repo.

If we pass it as a LDFLAG when building we can ensure it won't be changed.

Correct me if I'm wrong?

Version information such as tag, revision and build date and time is built at _build_ time without having to resort to using `GO_LDFLAGS` or `-ldflags`, see: `internal/build/build.go`.
@mbana
Copy link
Contributor Author

mbana commented May 20, 2022

I recommend we add version at build time just like I did in kusk-gayeway repo.

If we pass it as a LDFLAG when building we can ensure it won't be changed.

Correct me if I'm wrong?

I took another approach which is to call git and use the time package to partially get the version information. If you don't like it, let me know, and I'll change it. Here's where to look: https://github.com/kubeshop/kusk/pull/37/files#diff-f2ae1f6d37751ba11f66da69f40d1c3076bb1f2246878641080d5855034881b3R27.

internal/build/build.go Outdated Show resolved Hide resolved
Define a `version` command that uses value of `github.com/kubeshop/kusk-gateway/pkg/build` populated by `ldflags`:

* `github.com/kubeshop/kusk-gateway`: Get latest version that includes `github.com/kubeshop/kusk-gateway/pkg/build` package.
* `github.com/kubeshop/kusk-gateway/pkg/build`: `Version` variable will contain the latest tag when built.
* `TELEMETRY_TOKEN`: Remove references to `ldflags`/build-arg that is not used.
* `.github/workflows/go.yml`: Remove `test-GoReleaser-build` as we can just run it locally using `make build-goreleaser`.
* `.goreleaser.yml`: Populate `KuskGAMeasurementID` based on secret/env.
* `.goreleaser.yml`: Populate `KuskGAApiSecret` based on secret/env.

Example run log
---------------

```sh
$ make build
Makefile:62: warning: undefined variable 'GA_ID'
Makefile:62: warning: undefined variable 'GA_SECRET'
go build -v -o ./kusk -ldflags="-X 'github.com/kubeshop/kusk-gateway/pkg/build.Version=v1.0.4-5-ga63345e' -X 'github.com/kubeshop/kusk-gateway/pkg/analytics.KuskGAMeasurementID=' -X 'github.com/kubeshop/kusk-gateway/pkg/analytics.KuskGAApiSecret='" ./main.go
$ ./kusk version
kusk version 1.0.4-5-ga63345e
https://github.com/kubeshop/kusk/releases/latest
$ ./kusk --version
kusk version 1.0.4-5-ga63345e
https://github.com/kubeshop/kusk/releases/latest
$ ./kusk -v
kusk version 1.0.4-5-ga63345e
https://github.com/kubeshop/kusk/releases/latest
```
@mbana mbana changed the title Kusk CLI version - Partially closes kubeshop/kusk-gateway#418 Kusk CLI version - Partially closes kubeshop/kusk-gateway#415 May 23, 2022
Copy link
Contributor

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

while at it could you just change go.mod to go 1.18

.github/workflows/go.yml Show resolved Hide resolved
.goreleaser.yml Outdated Show resolved Hide resolved
@mbana
Copy link
Contributor Author

mbana commented May 23, 2022

while at it could you just change go.mod to go 1.18

This is quite risky. Wouldn't it require anyone doing a go install of kusk to have go1.18?

@jasmingacic
Copy link
Contributor

I'd ditch go install as an install option

@mbana
Copy link
Contributor Author

mbana commented May 23, 2022

I'd ditch go install as an install option

I can do it as a separate PR ... ? What do you think?

@mbana mbana requested a review from jasmingacic May 23, 2022 13:20
Copy link
Collaborator

@ypoplavs ypoplavs left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

Ship it!

…usk-gateway#415

> agreed. Just check if it contains the version

Just check the buffer contains the `version` in `Test_NewVersionCommand`.
@jasmingacic
Copy link
Contributor

go ahead and merge it

…usk-gateway#415

`.goreleaser.yml`: Strip binary of debug information (`LD_FLAGS += -w -s`) etc. - reduces the size of final download.
@mbana mbana merged commit 0823db0 into main May 23, 2022
@mbana mbana deleted the mbana-kusk-cli-version-command branch May 23, 2022 14:29
@mbana
Copy link
Contributor Author

mbana commented May 23, 2022

Tested with:

./kusk version; ./kusk --version; ./kusk -v             
kusk version 1.0.5-rc1
https://github.com/kubeshop/kusk/releases/tag/v1.0.5-rc1
kusk version 1.0.5-rc1
https://github.com/kubeshop/kusk/releases/tag/v1.0.5-rc1
kusk version 1.0.5-rc1
https://github.com/kubeshop/kusk/releases/tag/v1.0.5-rc1

Which seems right to me.

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.

Kusk CLI "version"
4 participants