🐛bugfix for operator-controller not outputting the right commit ID in the version #1811
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1811 +/- ##
==========================================
- Coverage 68.50% 68.45% -0.05%
==========================================
Files 63 63
Lines 5134 5136 +2
==========================================
- Hits 3517 3516 -1
- Misses 1388 1390 +2
- Partials 229 230 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
If this is a downstream-only concern, is it possible to make a change in the downstream repo instead? I'm kinda curious why the existing upstream implementation doesn't "just work" in downstreams since it should be pulling commit refs for the downstream git repo in the same way. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@joelanford yeah, this is a downstream only concern. And I think it's fine when we build from the repo downstream too, but it's something to do with the way images get generated using ART. That's when it's using the env var We can fix this in downstream repo directly as well without fixing it here, but I thought fixing it here and sync'ing it downstream would be better to have both repos as close as possible for maintainability purposes. Let me know if you disagree. |
|
@camilamacedo86 since we were discussing this on slack, will continue the conversation there to avoid typing in two different places :) |
rashmigottipati
force-pushed
the
fix-commit-ID
branch
from
0cb7ec3 to
47f389a
Compare
Makefile
Outdated
| GIT_COMMIT := $(if $(SOURCE_GIT_COMMIT),$(SOURCE_GIT_COMMIT),$(shell git rev-parse HEAD)) | ||
|
|
There was a problem hiding this comment.
I don't think we need this line. If we (or a fork) want to build binaries with a specific GIT_COMMIT reference, we/they should explicitly set GIT_COMMIT
When GIT_COMMIT is left unset, we will also set $(VERSION_PATH).gitCommit to empty string. So we should:
There was a problem hiding this comment.
Addressed based on this feedback in the latest commit.
This approach seems better as it eliminates the need to introduce an openshift specific envvar upstream.
camilamacedo86
dismissed
their stale review
I marked as solved my above comment #1811 (review) to make it easier for the review since we had already sorted out the raised concerns, and we will move with the change for upstream.
…version Signed-off-by: Rashmi Gottipati <[email protected]>
rashmigottipati
force-pushed
the
fix-commit-ID
branch
3 times, most recently
from
2dc7dad to
7afc14c
Compare
Signed-off-by: Rashmi Gottipati <[email protected]>
rashmigottipati
force-pushed
the
fix-commit-ID
branch
from
7afc14c to
6217d52
Compare
| @@ -29,7 +29,9 @@ func init() { | |||
| for _, setting := range info.Settings { | |||
| switch setting.Key { | |||
| case "vcs.revision": | |||
| gitCommit = setting.Value | |||
| if gitCommit == "" { | |||
There was a problem hiding this comment.
Couldn't this be a check against "unknown", and leave the default value as-is?
There was a problem hiding this comment.
i.e. is there a need to change the default?
Description
The
--versiondoesn't output the source code commit ID. It refers to "vcs-ref" in the image instead of "io.openshift.build.commit.id".Setting the
GIT_COMMITvalue toSOURCE_GIT_COMMITif empty (which is a corresponding downstream variable) will ensure that the version outputs the right commit ID as the code from this repo gets copied to another repo for building purposes.Reviewer Checklist