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

Jump to bottom

Bug 1715001: hack/build: Use BUILD_VERSION if non-empty #1829

Merged
merged 1 commit into from Aug 29, 2019
Merged

Bug 1715001: hack/build: Use BUILD_VERSION if non-empty #1829

merged 1 commit into from Aug 29, 2019

Conversation

wking
Copy link
Member

@wking wking commented Jun 6, 2019

Like 3313c08 (#1744), but for tags. This should fix #1828:

$ ./openshift-install version
./openshift-install v4.1.0-201905212232-dirty
...

Doozer sets SOURCE_GIT_TAG just like it sets SOURCE_GIT_COMMIT.

Fixes #1828.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 6, 2019
@wking
Copy link
Member Author

wking commented Jun 6, 2019

/hold

@sosiouxme is running this through doozer to see if it works, and we'll want to double-check the CI results to make sure the CI pipeline is handling it well too.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2019
@sosiouxme
Copy link
Member

doozer sets SOURCE_GIT_TAG using the tags from the source repo.
We need an env var that expresses the version and release being built (currently only set in the image labels).

@trown
Copy link

trown commented Jun 7, 2019

/test e2e-openstack

@wking
Copy link
Member Author

wking commented Jun 9, 2019

Looks like this is working in CI (by not changing anything vs. our previous code):

$ curl -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/1829/pull-ci-openshift-installer-master-e2e-aws/5977/artifacts/e2e-aws/installer/.openshift_install.log | head -n1
time="2019-06-06T20:00:26Z" level=debug msg="OpenShift Installer unreleased-master-1107-g6d4fda00d50911982ba2dfbfabc7930757ca8122-dirty"

We still need to figure out what variable doozer will set to hold the version being built.

@sosiouxme
Copy link
Member

We still need to figure out what variable doozer will set to hold the version being built.

@wking BUILD_VERSION and BUILD_RELEASE as it turns out.

http://pkgs.devel.redhat.com/cgit/containers/ose-installer/tree/Dockerfile?h=rhaos-4.1-rhel-7

@wking wking changed the title hack/build: Use SOURCE_GIT_TAG if set hack/build: Use BUILD_VERSION or BUILD_RELEASE if non-empty Aug 1, 2019
@wking
Copy link
Member Author

wking commented Aug 1, 2019

I've pushed 4932bd0 -> 59e43a3 to move us over to BUILD_VERSION and BUILD_RELEASE.

@wking
Copy link
Member Author

wking commented Aug 1, 2019

e2e-aws had the external API load-balancer throttled:

level=error msg="Error: timeout while waiting for state to become 'active' (last state: 'provisioning', timeout: 10m0s)" level=error level=error msg=" on ../tmp/openshift-install-451751868/vpc/master-elb.tf line 19, in resource \"aws_lb\" \"api_external\":" level=error msg=" 19: resource \"aws_lb\" \"api_external\" {"

/test e2e-aws

@wking
Copy link
Member Author

wking commented Aug 1, 2019

CI is still happily unaffected:

$ curl --compressed -s https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_installer/1829/pull-ci-openshift-installer-master-e2e-aws/7008/artifacts/e2e-aws/installer/.openshift_install.log | head -n1
time="2019-08-01T19:05:51Z" level=debug msg="OpenShift Installer unreleased-master-1497-g7177dc92ef38ed7c4d8e1c144bd584736ee65def-dirty"

@sosiouxme, did you want to wash this through doozer to confirm it picks up your BUILD_VERSION or BUILD_RELEASE?

sosiouxme
sosiouxme reviewed Aug 2, 2019
View reviewed changes
hack/build.sh Outdated Show resolved Hide resolved
@wking
hack/build: Use BUILD_VERSION if non-empty
dd7cff3 
Like 3313c08 (hack/build: Use SOURCE_GIT_COMMIT if set, 2019-05-13, openshift#1828).
This should fix [1]:

  $ ./openshift-install version
  ./openshift-install v4.1.0-201905212232-dirty
  ...

which is from Git looking at the build directory (and seeing -dirty
because doozer is adjusting our Dockerfile?).  Since the tags aren't
in the source repository (github.com/openshift/install), we need to
use the BUILD_* variables which were added to Doozer in [2].  In a
recent build [3], these looked like:

  ENV SOURCE_GIT_COMMIT=8aa5b10aa82d201deba0befbfac9fabf76f719f4 SOURCE_DATE_EPOCH=1561485623 BUILD_VERSION=v4.1.9 SOURCE_GIT_URL=https://github.com/openshift/installer SOURCE_GIT_TAG=8aa5b10a BUILD_RELEASE=201907311355

With this commit, we prefer BUILD_VERSION, falling back to 'git
describe' in the local repository if BUILD_VERSION is unset or empty.

Doozer also sets BUILD_RELEASE, but Luke says we always set
BUILD_VERSION in OpenShift 4 so there's no need to include it in the
fallback chain [4] (otherwise I'd have preferred BUILD_VERSION,
falling back to BUILD_RELEASE, falling back to Git inspection).

I don't see a need to use:

  GIT_TAG="${BUILD_VERSION}-${BUILD_RELEASE}"

with both, because if we know the version is v4.1.8, who cares about
the timestamp?  We only ever get a single 4.1.8 far enough along to
show up in front of end-users, even if there's a build hiccup or some
such that causes us to build multiple rounds with the same version
internally.  There's a bunch of v4.2.0, etc., in the run-up to a new
minor release, but we have commit hashes to distinguish between those.

[1]: openshift#1828
[2]: https://gitlab.cee.redhat.com/openshift-art/tools/doozer/commit/b02114a0bb26bbb7f60f1cc484c17fc3172c6df4
     distgit.py: ART-165 add build ENV vars, 2019-07-07
[3]: http://pkgs.devel.redhat.com/cgit/containers/ose-installer/tree/Dockerfile?h=rhaos-4.1-rhel-7
[4]: openshift#1829 (comment)
@wking
Copy link
Member Author

wking commented Aug 2, 2019

I'm comfortable with the test results, even though those were against 59e43a3 and I've since pushed further updates.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2019
@wking wking changed the title hack/build: Use BUILD_VERSION or BUILD_RELEASE if non-empty hack/build: Use BUILD_VERSION if non-empty Aug 2, 2019
@wking wking changed the title hack/build: Use BUILD_VERSION if non-empty Bug 1715001: hack/build: Use BUILD_VERSION if non-empty Aug 9, 2019
@openshift-ci-robot
Copy link
Contributor

@wking: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1715001: hack/build: Use BUILD_VERSION if non-empty

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 9, 2019
@sdodson
Copy link
Member

sdodson commented Aug 16, 2019

@patrickdillon or @vrutkovs PTAL

@vrutkovs
Copy link
Member

/retest

vrutkovs
vrutkovs approved these changes Aug 28, 2019
View reviewed changes
Copy link
Member

@vrutkovs vrutkovs 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 Aug 28, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrutkovs, wking

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:

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

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 5b714fe into openshift:master Aug 29, 2019
@openshift-ci-robot
Copy link
Contributor

@wking: All pull requests linked via external trackers have merged. Bugzilla bug 1715001 has been moved to the MODIFIED state.

In response to this:

Bug 1715001: hack/build: Use BUILD_VERSION if non-empty

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wking wking deleted the source-git-tag branch August 29, 2019 13:37
jhixson74 pushed a commit to jhixson74/installer that referenced this pull request Dec 6, 2019
@wking @jhixson74
hack/build: Use BUILD_VERSION if non-empty
8bc1c27 
Like 3313c08 (hack/build: Use SOURCE_GIT_COMMIT if set, 2019-05-13, openshift#1828).
This should fix [1]:

  $ ./openshift-install version
  ./openshift-install v4.1.0-201905212232-dirty
  ...

which is from Git looking at the build directory (and seeing -dirty
because doozer is adjusting our Dockerfile?).  Since the tags aren't
in the source repository (github.com/openshift/install), we need to
use the BUILD_* variables which were added to Doozer in [2].  In a
recent build [3], these looked like:

  ENV SOURCE_GIT_COMMIT=8aa5b10aa82d201deba0befbfac9fabf76f719f4 SOURCE_DATE_EPOCH=1561485623 BUILD_VERSION=v4.1.9 SOURCE_GIT_URL=https://github.com/openshift/installer SOURCE_GIT_TAG=8aa5b10a BUILD_RELEASE=201907311355

With this commit, we prefer BUILD_VERSION, falling back to 'git
describe' in the local repository if BUILD_VERSION is unset or empty.

Doozer also sets BUILD_RELEASE, but Luke says we always set
BUILD_VERSION in OpenShift 4 so there's no need to include it in the
fallback chain [4] (otherwise I'd have preferred BUILD_VERSION,
falling back to BUILD_RELEASE, falling back to Git inspection).

I don't see a need to use:

  GIT_TAG="${BUILD_VERSION}-${BUILD_RELEASE}"

with both, because if we know the version is v4.1.8, who cares about
the timestamp?  We only ever get a single 4.1.8 far enough along to
show up in front of end-users, even if there's a build hiccup or some
such that causes us to build multiple rounds with the same version
internally.  There's a bunch of v4.2.0, etc., in the run-up to a new
minor release, but we have commit hashes to distinguish between those.

[1]: openshift#1828
[2]: https://gitlab.cee.redhat.com/openshift-art/tools/doozer/commit/b02114a0bb26bbb7f60f1cc484c17fc3172c6df4
     distgit.py: ART-165 add build ENV vars, 2019-07-07
[3]: http://pkgs.devel.redhat.com/cgit/containers/ose-installer/tree/Dockerfile?h=rhaos-4.1-rhel-7
[4]: openshift#1829 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sosiouxme sosiouxme sosiouxme left review comments

@vrutkovs vrutkovs vrutkovs approved these changes

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

Assignees

@vrutkovs vrutkovs

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v4.1.0 reports a dirty version
8 participants
@wking @sosiouxme @trown @openshift-ci-robot @sdodson @vrutkovs @openshift-bot @openshift-merge-robot