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

stopped aborting startup when in container/k8s if site props is r/o #1646

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

artntek
Copy link
Contributor

@artntek artntek commented Jun 22, 2023

stopped aborting startup when in container/k8s if site props is r/o (expected in that environment)

Copy link
Contributor

@taojing2002 taojing2002 left a comment

Choose a reason for hiding this comment

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

It looks good!

Copy link
Member

@mbjones mbjones left a comment

Choose a reason for hiding this comment

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

Took a quick look, overall looks good.

In the comments you state that this checks if /var/metacat is writable, but I don't immediately see where that is checked. It would be good to check it, as even in k8s we need that writable volume to be in place for data to be written.

@artntek
Copy link
Contributor Author

artntek commented Jun 22, 2023

In the comments you state that this checks if /var/metacat is writable, but I don't immediately see where that is checked. It would be good to check it, as even in k8s we need that writable volume to be in place for data to be written.

Unfortunately, the comments are misleading. When the app is starting up, we don't know for sure that /var/metacat/ is the location for the metacat files (since that's configurable). (We do know it's /var/metacat in k8s, because that's hardcoded into the image, but we also create those directories in docker-entrypoint.sh, thus guaranteeing they are there (or startup fails).

If you want me to add validation for /var/metacat being writeable only when running in a container, I'm happy to do that, but since that's checked elsewhere, a simpler approach would be to change the comments so they aren't misleading, and leave the code as is.

LMK which you prefer.

@mbjones
Copy link
Member

mbjones commented Jun 22, 2023

Fixing the comments for now is fine. As you say in that class documentation, there are more checks to be added later.

@artntek artntek merged commit a6b4b44 into develop Jun 23, 2023
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.

3 participants