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

[1.18] Revert the auto-injecting version changes #3613

Merged
merged 1 commit into from Apr 20, 2020

Conversation

umohnani8
Copy link
Member

Signed-off-by: Urvashi Mohnani umohnani@redhat.com

What type of PR is this?

/kind design

What this PR does / why we need it:

Reverting this so we don't run into build issues with
openshift again. We will revisit in master to come up with
a less fragile way of doing this.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


Signed-off-by: Urvashi Mohnani umohnani@redhat.com

@openshift-ci-robot openshift-ci-robot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Apr 19, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umohnani8

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2020
Reverting this so we don't run into build issues with
openshift again. We will revisit in master to come up with
a less fragile way of doing this.

Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #3613 into release-1.18 will not change coverage.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##           release-1.18    #3613   +/-   ##
=============================================
  Coverage         44.01%   44.01%           
=============================================
  Files               102      102           
  Lines              7827     7827           
=============================================
  Hits               3445     3445           
  Misses             4075     4075           
  Partials            307      307           

@mrunalp
Copy link
Member

mrunalp commented Apr 19, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. labels Apr 19, 2020
@cgwalters
Copy link
Contributor

Thanks for the revert; this is stalling RHCOS builds and we're trying to get some other things landed too.

I think gathering version information from git is useful as an informational thing, but it gets fragile if one is trying to actively make decisions based on it. Even without the weird "lookaside cache" case where Koji does that breaks any relationship with the upstream git repo. For example, it's reasonable for downstream project consumers to make their own git repositories and cherry pick patches there - and they shouldn't have to also manage syncing up tags and such.

It's just more robust to do const version = "1.18" in the source code and the cost of doing that is low.

But, it definitely also makes sense to record the git commit, I just wouldn't use it for things like crio-wipe.

@cgwalters
Copy link
Contributor

I also tried setting VERSION=%{version} in our RPM spec build, and I swear it worked once (I verified that ./crio version had a version again) but then failed (still had no version tag) when I tried it a second time...didn't try too hard to debug it though.

@mrunalp
Copy link
Member

mrunalp commented Apr 19, 2020

/retest

@mrunalp
Copy link
Member

mrunalp commented Apr 19, 2020

@lsm for new build once this merges.

@mrunalp
Copy link
Member

mrunalp commented Apr 19, 2020

/test e2e-aws

@openshift-merge-robot openshift-merge-robot merged commit 2cef6c3 into cri-o:release-1.18 Apr 20, 2020
This was referenced May 12, 2020
@umohnani8 umohnani8 deleted the 1.18-version branch July 23, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants