⚠Provide version information as a CLI option #1773

Adirio · 2020-11-03T08:14:07Z

Description

Provide a WithVersion CLI Option that allows to provide a version string instead of having to create a version command and provide it as an extra command. When a version is provided this way, the CLI will create a version command automatically that prints this version string.

Changes

✨ New argument type for cli.New: WithVersion. Creating a command and providing it with WithExtraCommands should still work with the previous behavior, so this is not a breaking change, but frameworks using kubebuilder should migrate to the new approach.
⚠️ Removed Version and NewCmd from package cmd/version. This package is not part of the library itself, but as it is exported, it may be used by third parties.

Adirio · 2020-11-03T08:31:03Z

@camilamacedo86 This PR goes in the same direction as #1691, moving another command out of the extra commands. I think that the API for frameworks with the new options is clearer, they only have to provide a string now instead of a full COBRA command, and it also has the added benefot of including kubebuilder --version and kubebuilder -v in addition to kubebuilder version. WDYT?

Adirio · 2020-11-04T06:42:17Z

A note in coverage: this PR moves part of the remaining code from cmd to pkg. As cmd was not being checked by coveralls, it says that the coverage has decreased, but in reality we just have included in the covered package part of the code that wasn't being covered by coveralls.

cmd/main.go

camilamacedo86

I am ok with. 👍 I have just a nit. See; https://github.com/kubernetes-sigs/kubebuilder/pull/1773/files#r518800235
Otherwise:
/approve

@estroz do you see any reason for we do not move forward here? WDYT?

cmd/version/version.go

estroz · 2020-11-06T18:03:21Z

pkg/cli/cli.go

@@ -424,7 +442,7 @@ After the scaffold is written, api will run make on the project.
  make run
 `,
 			c.commandName, c.commandName),
-
+		Version: c.version,


According to the docs, this is only necessary if the command doesn't define a version command.

According to the docs this has to do with -v and --version flags, not with version command:

// Version defines the version for this command. If this value is non-empty and the command does not
// define a "version" flag, a "version" boolean flag will be added to the command and, if specified,
// will print content of the "Version" variable. A shorthand "v" flag will also be added if the
// command does not define one.

Ah I read that wrong. I don't think we want to expose a --version and a version command though.

I don't see any damage done by being able to do kubebuilder version, kubebuilder --version and kubebuilder -v. I think it actually improves UX as it doesn't have to know if it is a command or a flag. kubebuilder help and kubebuilder --help have the same behavior, right?

pkg/cli/version.go

Adirio · 2020-11-10T12:37:58Z

@estroz can we merge this? The only remaining comment is that you wouldn't add kubebuilder --version in addition to kubebuilder version but I don't see any harm indoing so. Do you have any reason to say so?

estroz · 2020-11-10T18:39:58Z

@Adirio the only hesitation I have is that most k8s tools like kubectl only have the version command, and I'd prefer to adhere to that standard in kubebuilder. Also this reserves -v for verbose mode, which is also common.

Adirio · 2020-11-10T20:27:40Z

@estroz Removed the --version and -v flags.

P.S.: if we set a -v flag for verbose, setting Version won't create that flag.

Signed-off-by: Adrian Orive <[email protected]>

estroz · 2020-11-10T22:10:29Z

/retest

estroz · 2020-11-10T22:18:44Z

common.sh

-    # TODO: what does this thing do.
-    opts=-ldflags "-X sigs.k8s.io/kubebuilder/v2/cmd/version.kubeBuilderVersion=$INJECT_KB_VERSION"
+    # Injects the version into the cmd/version.go file
+    opts=-ldflags "-X sigs.k8s.io/kubebuilder/v2/cmd.kubeBuilderVersion=$INJECT_KB_VERSION"


Wdyt about adding adding something like this to make build, ex.

BUILD_VERSION=$(shell git descripe --dirty --tags --always) BUILD_COMMIT=$(shell git rev-parse HEAD) LD_FLAGS=-ldflags "-X sigs.k8s.io/kubebuilder/v2/cmd.kubeBuilderVersion=$(BUILD_VERSION) -X sigs.k8s.io/kubebuilder/v2/cmd.gitCommit=$(BUILD_COMMIT)" build: ## Build the project locally go build $(LD_FLAGS) -o bin/kubebuilder ./cmd

perhaps in a follow-up?

@estroz I tracked this change in an Issue some time ago: #1401
goos, goarch and buildDate should also be doable.

estroz

/lgtm

k8s-ci-robot · 2020-11-10T22:19:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, camilamacedo86, estroz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86,estroz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from camilamacedo86 and joelanford November 3, 2020 08:14

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 3, 2020

Adirio mentioned this pull request Nov 3, 2020

✨ Provide a cli option to enable and disable completion command #1776

Merged

Adirio force-pushed the version-cmd branch from 05a22c3 to a413ba2 Compare November 4, 2020 14:02

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 4, 2020

Adirio force-pushed the version-cmd branch from a413ba2 to ea92c92 Compare November 4, 2020 14:17

camilamacedo86 reviewed Nov 4, 2020

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020

Adirio force-pushed the version-cmd branch from ea92c92 to ce0425d Compare November 4, 2020 19:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2020

Adirio closed this Nov 5, 2020

Adirio reopened this Nov 5, 2020

Adirio force-pushed the version-cmd branch from ce0425d to 621ba56 Compare November 5, 2020 11:16

camilamacedo86 approved these changes Nov 6, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2020

camilamacedo86 reviewed Nov 6, 2020

View reviewed changes

cmd/version/version.go Show resolved Hide resolved

camilamacedo86 requested a review from estroz November 6, 2020 14:52

estroz reviewed Nov 6, 2020

View reviewed changes

pkg/cli/version.go Outdated Show resolved Hide resolved

Adirio force-pushed the version-cmd branch from 621ba56 to c8eaf36 Compare November 6, 2020 18:16

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2020

Adirio force-pushed the version-cmd branch from c8eaf36 to 16ee059 Compare November 8, 2020 21:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2020

Provide version information as a CLI option

8c51104

Signed-off-by: Adrian Orive <[email protected]>

Adirio force-pushed the version-cmd branch from 16ee059 to 8c51104 Compare November 10, 2020 20:32

estroz reviewed Nov 10, 2020

View reviewed changes

estroz approved these changes Nov 10, 2020

View reviewed changes

k8s-ci-robot assigned estroz Nov 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020

k8s-ci-robot merged commit c158f4f into kubernetes-sigs:master Nov 11, 2020

Adirio deleted the version-cmd branch November 11, 2020 07:23

Adirio mentioned this pull request Nov 11, 2020

Build-related flags for executables built from source #1401

Closed

camilamacedo86 mentioned this pull request Feb 18, 2021

New Release - proposing 3.0.0 stable #2029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠Provide version information as a CLI option #1773

⚠Provide version information as a CLI option #1773

Adirio commented Nov 3, 2020 •

edited

Loading

Adirio commented Nov 3, 2020

Adirio commented Nov 4, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

estroz Nov 6, 2020

Adirio Nov 6, 2020

estroz Nov 6, 2020

Adirio Nov 7, 2020

Adirio commented Nov 10, 2020

estroz commented Nov 10, 2020

Adirio commented Nov 10, 2020 •

edited

Loading

estroz commented Nov 10, 2020

estroz Nov 10, 2020

Adirio Nov 11, 2020 •

edited

Loading

estroz left a comment

k8s-ci-robot commented Nov 10, 2020

⚠Provide version information as a CLI option #1773

⚠Provide version information as a CLI option #1773

Conversation

Adirio commented Nov 3, 2020 • edited Loading

Description

Changes

Adirio commented Nov 3, 2020

Adirio commented Nov 4, 2020 • edited Loading

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

estroz Nov 6, 2020

Choose a reason for hiding this comment

Adirio Nov 6, 2020

Choose a reason for hiding this comment

estroz Nov 6, 2020

Choose a reason for hiding this comment

Adirio Nov 7, 2020

Choose a reason for hiding this comment

Adirio commented Nov 10, 2020

estroz commented Nov 10, 2020

Adirio commented Nov 10, 2020 • edited Loading

estroz commented Nov 10, 2020

estroz Nov 10, 2020

Choose a reason for hiding this comment

Adirio Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 10, 2020

Adirio commented Nov 3, 2020 •

edited

Loading

Adirio commented Nov 4, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

Adirio commented Nov 10, 2020 •

edited

Loading

Adirio Nov 11, 2020 •

edited

Loading