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

[bitnami/minio] Update minio-env.sh to allow custom path for persist #55562

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

Mo0rBy
Copy link
Contributor

@Mo0rBy Mo0rBy commented Jan 29, 2024

…nce directory and updated README.md

Description of the change

This change allows the MINIO_DATA_DIR environment variable to be a configurable value so that the persistence directory can exist at a path inside the container that is not /bitnami/minio/data.

Benefits

This allows users to have a persistence directory that is not the default bitnami/minio/data path so that mounting the persistence volume to something like AWS EFS or EBS can be done with naming conventions suited to the user/team.

Possible drawbacks

This is very small change, I honestly can't think of any apart from something to check while troubleshooting/debugging.

Applicable issues

My own issue created in bitnami/charts > bitnami/charts#21757 (I will be looking at creating a Helm chart PR in the next few days as well, when I get time).

Additional information

I tested this by running the container image locally, going into the UI and creating a new bucket called test-bucket and you can see that when I shell into the container, there is a test-bucket directory created at the custom persistence path.

Here is a screenshot of my terminal to prove that the bucket exists in the correct place (inside custom-data-dir):

Screenshot from 2024-01-29 22-36-15

Screenshot from 2024-01-29 22-37-20

I would not consider this a limitation but this configuration requires that the user deploying the container must have the container path mount and the environment variables as the same value. This is ONLY when configuring the container to use a custom persistence path, the default /bitnami/minio/data can still be used. I have updated the relevant section of the README.md which explains.

@github-actions github-actions bot added minio triage Triage is needed labels Jan 29, 2024
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Jan 30, 2024
@github-actions github-actions bot removed the triage Triage is needed label Jan 30, 2024
@github-actions github-actions bot removed the request for review from carrodher January 30, 2024 08:29
Copy link
Contributor

@FraPazGal FraPazGal left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution @Mo0rBy! The PR looks good on my side, though I added a minor comment as review. Please check it whenever you can!

@@ -78,6 +78,7 @@ export MINIO_DISTRIBUTED_MODE_ENABLED="${MINIO_DISTRIBUTED_MODE_ENABLED:-no}"
export MINIO_DEFAULT_BUCKETS="${MINIO_DEFAULT_BUCKETS:-}"
export MINIO_STARTUP_TIMEOUT="${MINIO_STARTUP_TIMEOUT:-10}"
export MINIO_SERVER_URL="${MINIO_SERVER_URL:-$MINIO_SCHEME://localhost:$MINIO_API_PORT_NUMBER}"
export MINIO_DATA_DIR="${MINIO_DATA_DIR:-/bitnami/minio/data}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick; could you move it back to the path section? It's true it could also be considered part of MinIO config but having all the path-related envs grouped seems more intuitive to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I was 50/50 on where this should go in here as everything in the "path" section is non-configurable, but this is still a path, but now it is a configurable path!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FraPazGal Bumping this

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, the PR LGTM @Mo0rBy! Let me run a couple of check in our internal pipeline before merging this.

…nce directory and updated README.md

Signed-off-by: Will Moorby <will.moorby2@gmail.com>
@Mo0rBy Mo0rBy requested a review from FraPazGal February 2, 2024 09:23
Copy link
Contributor

@FraPazGal FraPazGal left a comment

Choose a reason for hiding this comment

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

LGTM!

@FraPazGal FraPazGal merged commit 27e3e40 into bitnami:main Feb 7, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minio solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants