-
Notifications
You must be signed in to change notification settings - Fork 86
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
ZENKO-4905-check-patches-on-ci #2202
base: development/2.11
Are you sure you want to change the base?
ZENKO-4905-check-patches-on-ci #2202
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
7978d27
to
728074e
Compare
Previously the path for all charts was concidered as a single one ( not taking into concideration white spaces) causing the make patch command to fail, thus the intorduced loop to go through each chat path individually Issue: ZENKO-4905
bd04824
to
de06258
Compare
1c7ba72
to
acebd94
Compare
solution-base/mongodb/Makefile
Outdated
@@ -5,11 +5,10 @@ CHART_REPO:="https://charts.bitnami.com/bitnami" | |||
CHART_MONGO_SHARDED_VERSION:="7.9.1" | |||
|
|||
PATCH_DIR:="${ROOT_DIR}/patches" | |||
PATCH_FILES:="$(shell ls -d ${PATCH_DIR}/*)" | |||
|
|||
PATCH_FILES:=$(shell ls ${PATCH_DIR}/* | grep -v mongodb-sharded-add-configsvr-service-file.patch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not filter patches: if we need not to apply it, then we must actually remove it (git rm
)...
PATCH_FILES:=$(shell ls ${PATCH_DIR}/* | grep -v mongodb-sharded-add-configsvr-service-file.patch) | |
PATCH_FILES:=$(shell ls ${PATCH_DIR}/*) |
ac87420
to
ada5dd2
Compare
solution-base/mongodb/Makefile
Outdated
@@ -5,8 +5,7 @@ CHART_REPO:="https://charts.bitnami.com/bitnami" | |||
CHART_MONGO_SHARDED_VERSION:="7.9.1" | |||
|
|||
PATCH_DIR:="${ROOT_DIR}/patches" | |||
PATCH_FILES:="$(shell ls -d ${PATCH_DIR}/*)" | |||
|
|||
PATCH_FILES:=$(shell ls ${PATCH_DIR}/*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Why this change?
run: make fetch-mongodb-sharded | ||
working-directory: ./solution-base/mongodb | ||
- name: remove configsrv file | ||
run: rm -vf charts/mongodb-sharded/templates/config-server/config-server-service.yaml || echo "Failed to remove" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to remove?
if this is needed, should be part of a patch...
(if helm does not does remove "external" files, then we may need remove the whole chart folder in make fetch-mongodb-sharded
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no the file isn't there when we fetch de mongo sharded , it has always been added manually , the chart creates it. That's why here I remove it otherwise i end up with an error saying that the file already exists ( I don't have the historic linked to this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is an issue in the patch file,
--- /dev/null
+++ b/solution-base/mongodb/charts/mongodb-sharded/templates/config-server/config-server-service.yaml
and should be fixed by updating the patchfile, not the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested:
- "updating" the charts does not actually remove files which were created... So upgrading "in-place" just overwrites the files, but leaves removed files in place...
- the patches work when we simply delete the charts directory before fetching: however, this causes the git diff to fail as we added some files:
supprimé : charts/mongodb-sharded/charts/common/values.schema.json supprimé : charts/mongodb-sharded/files/docker-entrypoint-initdb.d/create-app-user-sharded.sh supprimé : charts/mongodb-sharded/files/docker-entrypoint-initdb.d/set-default-write-concern-majority-sharded.sh supprimé : charts/mongodb-sharded/templates/initialization-configmap.yaml
- Regarding
config-server-service.yaml
, this file is indeed not present in the chart: not related to patching, but I wonder why we added it (or kept it), and if we need it still...
- clearing the folder before fetch is the way
- need to update the patch files to "create" these removed files/links
- need to review if we still need
values.schema.json
,initialization-configmap.yaml
andconfig--server-service.yaml
c841d7c
to
7ed8014
Compare
/approve |
/wait |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: approve |
7ed8014
to
5c600e1
Compare
Issue: ZENKO-4905