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

Feature: make data dir configurable #4

Conversation

ronaldyuwandika
Copy link

@ronaldyuwandika ronaldyuwandika commented Sep 30, 2020

This is PR for fix this issue #3

What I do:

  • add support to dynamically change the default CSI driver rawfile and data directory

How to tests:

  • Just change the value of rawfile_dir and data_dir in the helm values file stored at deploy/charts/rawfile-csi/values.yaml

Signed-off-by: ronaldyuwandika [email protected]

@ronaldyuwandika ronaldyuwandika changed the title Feature/make data dir configurable Feature: make data dir configurable Sep 30, 2020
@jshin47
Copy link

jshin47 commented Oct 2, 2020

The CI is failing but I don't think it has anything to do with your code...

@ronaldyuwandika
Copy link
Author

I think the CI is failing because there's a problem with the docker build and push stage that needs username and password for dockerhub repository

Copy link
Collaborator

@semekh semekh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Will you please fix the comments?

@@ -12,6 +12,10 @@ defaults: &defaults
requests:
cpu: 10m
memory: 100Mi
rawfile_dir: /rawfile
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to /var/csi/rawfile to keep it backward-compatible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I will change it later

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello, @semekh I've updated this to /var/csi/rawfile
can you please check it again?
thank you

Dockerfile Outdated
@@ -19,5 +19,7 @@ ENV PYTHONUNBUFFERED 1

ARG IMAGE_TAG
ARG IMAGE_REPOSITORY
ARG DATA_DIR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not make data_dir configurable. This is what the container sees, and does not need to be changed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, I'll undo this change

Copy link
Collaborator

@semekh semekh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fix these and it's good to go.

@@ -66,6 +66,8 @@ spec:
value: unix:///csi/csi.sock
- name: IMAGE_REPOSITORY
value: "{{ .Values.node.image.repository }}"
- name: DATA_DIR
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "remove data_dir variable" applies to this one too

@@ -85,7 +87,7 @@ spec:
mountPath: /var/lib/kubelet
mountPropagation: "Bidirectional"
- name: data-dir
mountPath: /data
mountPath: {{ .Values.node.data_dir }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -12,6 +12,10 @@ defaults: &defaults
requests:
cpu: 10m
memory: 100Mi
rawfile_dir: /var/csi/rawfile
data_dir: /data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -20,7 +20,7 @@ spec:
imagePullPolicy: IfNotPresent
volumeMounts:
- name: data-dir
mountPath: /data
mountPath: {data_dir}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -11,7 +11,7 @@ spec:
volumes:
- name: data-dir
hostPath:
path: /var/csi/rawfile
path: {rawfile_dir}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this needs to be passed into the templating function so that it can get rendered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
can i change it into templating function like this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values are interpolated not by helm, but the python code. So that's where the code needs to be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants