Skip to content

Makefile,internal/version: inject version based on git tree#3854

Merged
joelanford merged 1 commit intooperator-framework:masterfrom
joelanford:inject-version
Sep 10, 2020
Merged

Makefile,internal/version: inject version based on git tree#3854
joelanford merged 1 commit intooperator-framework:masterfrom
joelanford:inject-version

Conversation

@joelanford
Copy link
Copy Markdown
Member

@joelanford joelanford commented Sep 4, 2020

Description of the change:
Remove hard-coded version string in internal/version/version.go

This is an initial fully-backward compatible change for other code within the SDK that depends on this value (e.g. scaffolding image reference tags based on the SDK version). In a future PR, we could probably remove internal/version.Version entirely and depend only on internal/version.GitVersion

Motivation for the change:
We can automatically inject the correct string based on reading the git tree when building from the Makefile. This reduces the number of steps and manual changes that are necessary as part of the release process.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

}
fmt.Printf("operator-sdk version: %q, commit: %q, kubernetes version: %q, go version: %q, GOOS: %q, GOARCH: %q\n",
version, ver.GitCommit, ver.KubernetesVersion, ver.GoVersion, runtime.GOOS, runtime.GOARCH)
ver.GitVersion, ver.GitCommit, ver.KubernetesVersion, ver.GoVersion, runtime.GOOS, runtime.GOARCH)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It will start to use the dev version as unknown. Am I right?

we need to update the e2e tests See: https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-helm/e2e_helm_suite_test.go#L108-L109

Also, maybe the molecule shell as well.
Also, will the (+git) still or not? if not, then we need

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If the binary is built with the Makefile, this value will be the same as it has always been, because the Makefile will inject this value automatically.

Everything in the CI and release process uses the Makefile, so this shouldn't have any other impact.

Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 Sep 4, 2020

Choose a reason for hiding this comment

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

@joelanford,

The e2e tests as the shell scripts will run operator-sdk init to scaffold the projects. The version is used in the scaffolds (here).

Then, note that in the e2e test, for example, we replace the version.version+git by dev:https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-helm/e2e_helm_suite_test.go#L107-L110

See the CI error faced in this PR: (unable to build the image)

    s: "make docker-build IMG=e2e-test/controller-manager:tfcf failed with error: make[1]: Entering directory '/home/travis/gopath/src/github.com/operator-framework/operator-sdk/test/e2e-helm/e2e-tfcf'\ndocker build . -t e2e-test/controller-manager:tfcf\nSending build context to Docker daemon  35.87MB\r\r\nStep 1/5 : FROM quay.io/operator-framework/helm-operator:\ninvalid reference format\nMakefile:42: recipe for target 'docker-build' failed\nmake[1]: *** [docker-build] Error 1\nmake[1]: Leaving directory '/home/travis/gopath/src/github.com/operator-framework/operator-sdk/test/e2e-helm/e2e-tfcf'\n",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I need to debug that. Theoretically, the correct value should have been injected into that operator-sdk binary used to run operator-sdk init. I'll pick this back up and rebase after #3856 is resolved because that's blocking everything from merging right now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@camilamacedo86 this PR is now updated, and all tests are now passing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@joelanford it broke the tests. It is passing because the test has been done with the 1.0.0 tag instead of the dev one.
it is no longer using the dev image :-(

change the Replace in test/utils/utils.go

// ReplaceInFile replaces all instances of old with new in the file at path.
func ReplaceInFile(path, old, new string) error {
	info, err := os.Stat(path)
	if err != nil {
		return err
	}
	b, err := ioutil.ReadFile(path)
	if err != nil {
		return err
	}
	if !strings.Contains(string(b), old) {
		return errors.New("unable to find the content to be replaced")
	}
	s := strings.Replace(string(b), old, new, -1)
	err = ioutil.WriteFile(path, []byte(s), info.Mode())
	if err != nil {
		return err
	}
	return nil
}

You will see that it will return the error unable to find the content to be replaced in all e2e tests.

@joelanford joelanford force-pushed the inject-version branch 2 times, most recently from 3440100 to e410dd0 Compare September 10, 2020 00:21
Copy link
Copy Markdown
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2020
@joelanford joelanford merged commit 34c0fca into operator-framework:master Sep 10, 2020
@joelanford joelanford deleted the inject-version branch September 10, 2020 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants