build: use BUILD_TAGS instead of CEPH_VERSION
#2626
Conversation
Ceph base container images have predefined value for CEPH_VERSION env variable. Hence, use BUILD_TAGS as variable name instead to avoid mismatch. Signed-off-by: Rakshith R <rar@redhat.com>
mergify
bot
added
the
component/build
|
/retest ci/centos/mini-e2e-helm/k8s-1.20 |
failed due to #2264 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
rbd-nbd related failures |
|
/retest ci/centos/mini-e2e/k8s-1.20 |
|
/retest ci/centos/mini-e2e/k8s-1.22 |
Both failed due to #2610 |
There was a problem hiding this comment.
I do not understand why this is needed. The idea behind it is to use the CEPH_VERSION that the container provides. That is then used to construct the GO_TAGS for passing the right Ceph version (name) to go-ceph to limit/enable certain features. When setting a different BUILD_TAGS version, the container can have a different CEPH_VERSION that is not compatible.
Could you explain why the CEPH_VERSION from the container should not be used?
If this is the concern, add a comment above |
I hit this when trying to pass And I really did not think of this I can modify the pr to append |
|
Test: Logs: We have seen Input/Output errors before when the nodeplugin pod is restarted, which is anyway tracked with #2204 @Madhu-1 @nixpanic probably I'm missing who is attempting to do mkfs on /dev/nbd0 after nodeplugin restart? Not seen this before! |
|
@pkalever during nodeStage operation, if the device format is empty a cephcsi will format the device and mount it. |
Adding support for additional |
|
BUILD_TAGS makes more sense for golang build instead of CEPH_VERSION.
BUILD_TAGS makes more sense for golang build instead of CEPH_VERSION. cant we git rid of one more variable here? |
| ifndef CEPH_VERSION | ||
| CEPH_VERSION = $(shell . $(CURDIR)/build.env ; echo $${CEPH_VERSION}) | ||
| ifndef BUILD_TAGS | ||
| BUILD_TAGS = $(shell . $(CURDIR)/build.env ; echo $${BUILD_TAGS}) |
There was a problem hiding this comment.
may be call this as `CEPH_BUILD_TAGS" as it is referring to Ceph Build ?
There was a problem hiding this comment.
We do not need to prefix everything with CEPH_CSI_ or CEPH_ imho. BUILD_TAGS suggest any Golang build-tags can be included, which is the goal here.
We do need to consume the |
| ifndef CEPH_VERSION | ||
| CEPH_VERSION = $(shell . $(CURDIR)/build.env ; echo $${CEPH_VERSION}) | ||
| ifndef BUILD_TAGS | ||
| BUILD_TAGS = $(shell . $(CURDIR)/build.env ; echo $${BUILD_TAGS}) |
There was a problem hiding this comment.
We do not need to prefix everything with CEPH_CSI_ or CEPH_ imho. BUILD_TAGS suggest any Golang build-tags can be included, which is the goal here.
| @@ -6,7 +6,7 @@ | |||
| # options for analysis running | |||
| run: | |||
| build-tags: | |||
| - @@CEPH_VERSION@@ | |||
| - @@BUILD_TAGS@@ | |||
There was a problem hiding this comment.
can you pass octopus,ceph_preview here, or does it need to be a list like
build-tags:
- @@CEPH_VERSION@@
- ceph_preview
I guess adding ifndef CEPH_VERSION
CEPH_VERSION = $(shell . $(CURDIR)/build.env ; echo $${CEPH_VERSION})
endif
ifdef CEPH_VERSION
# pass -tags to go commands (for go-ceph build constraints)
GO_TAGS = -tags=$(CEPH_VERSION)
endif
ifdef BUILD_TAGS
# additional build tags
GO_TAGS = GO_TAGS,$(BUILD_TAGS)
endif
In golangci.yaml.in this sounds better? |
why we want it to pick up from the base image as we are expertly passing when building the image? am I missing something here? |
from @nixpanic comment here #2626 (comment)
This part sounds very tedious, : exactly locating |
|
closing this pr, since the proposed change here is introduced as part of #2633 |
Ceph base container images have predefined value
for CEPH_VERSION env variable. Hence, use BUILD_TAGS
as variable name instead to avoid mismatch.
Signed-off-by: Rakshith R rar@redhat.com
For example, in build run https://github.com/ceph/ceph-csi/runs/4144225306?check_suite_focus=true
build tag is set as
pacificwhich should have beenoctopussourced frombuild.envfile