Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: WP-813 CMS customization fails, CMS is old #1034
base: main
Are you sure you want to change the base?
fix: WP-813 CMS customization fails, CMS is old #1034
Changes from 6 commits
5472b9d
9d91f9e
f8e07b8
62c286b
0faa912
fa70269
b11800a
59a72f8
3e299d0
35e0a36
a5379e9
4c0fb05
985e86a
699334f
3c0187b
a4538a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
99% of use of the CMS in this repo by devs is just to have a homepage. I think a good goal would be to have a default config (the existing
secrets.sample.py
) to get up and running immediately, and if need be a dev who needs bespoke CMS settings can set them after the fact. Think we can figure something out?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.
I had thought so, and am fine with minimal working secrets (as it was).
@taoteg reports devs adding CMS non-secret settings to CMS
secrets.py
. Or maybe I misunderstood. For fear that is happening:settings_custom.py
andsettings_local.py
.settings_custom.py
andsettings_local.py
being added as directories if they are absent, I added commands to create them.curl
ed them from TACC/Core-CMS.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.
Personally I think keeping a working set of default settings committed makes more sense for local dev, and updates as needed can be committed to the repo.
How about this: We commit a set of local settings that are committed with dummy credentials that will be imported into the CMS settings on make start, and if a (untracked)
secrets.py
happens to exist, then that acts as an overrideThere 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.
I am looking for parity between env setups here. We should be using the same config files (settings, settings_custom, settings_local, secrets) in every env (dev, local, pprd, prod).
By leaving out parts of the default config structure we are not training devs on the expected practice for how these files exist in pprd/prod and why certain settings exist in certain files.
Examples:
SILENCED_SYSTEM_CHECKS = ['captcha.recaptcha_test_key_error']
in the example settings. This error will creep up in the logging (annoying). [Supposedly this will go away whenFP-1728: Remove once servers have RECAPTCHA_..._KEY settings
but so far has not for me.. maybe due to the old CMS image?]. This setting should go in settings_local, but how would you do that in the CMS with the current setup? By overloading the secrets file... which is bad practice.This request is as much about consistency in workflows as it is about having good settings out of the box that use a current CMS image.
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.
Ah, the
SILENCED_SYSTEM_CHECKS
is in Core-CMSsettings_local.py
…. Another way to resolve that missing piece is add it to a committed and trackedsettings_custom.py
(which @rstijerina proposed to us privately).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.
New solution available via
settings_default.py
(which Core-CMS supports in TACC/Core-CMS#904.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.
This allows
settings_local.py
andsettings_custom.py
to function. I did not add:ro
, because (based on my chat with Docker's AI) that will not prevent directory from showing if files exist.Details
These are methods to prevent directory from being created if single-file volume is missing:
external: true
; this causes error instead of empty directory.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.
In Production the :ro ensures that when a new portal VM is provisioned and configured if any of the placed files are forgotten (secrets, settings_local) the portal's docker engine will not generate an empty directory and try to mount said empty dir, docker will instead complain about the missing files - which is what we want. You may still want the mount to fail in local dev if the files are not present (ie - were not curled or copied as needed)? Its a quick way of knowing you missed something. Thoughts?
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.
A. Sounds fine, but adding
:ro
does not have that effect for me locally. Are my test steps flawed?Test Steps
:ro
to each settings & secrets file volume.make stop
.make start
.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.
B. I tried using verbose syntax, so I could change the mount type to
bind
. This change, withread_only: true
worked i.e. caused error if a file is missing.Test Steps
:ro
to each settings & secrets file volume.make stop
.(from testing method A)
make start
.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.
C. I investigated again the other method I'd read, a named volume with
external: true
. It requires manually adding a volume first and an equally verbose docker-compose file change. And I don't think it will work (because how would container tell CMS where file is).Instructions (Untested)
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.
@rstijerina & @taoteg, I do not want to start Idea D yet.1
This PR, as of latest commit 4c0fb05, would create directories. But, anyone who would customize the CMS, is instructed to delete them, touch the files, restart, and populate the settings as necessary.
Is this sufferable for now?
Footnotes
Because it requires refactor in Core-CMS, clients and developers to move their settings, and Camino to update where it places CMS settings. ↩
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.
How about your verbose method, that doesn't create empty dirs?
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.
B.2. Ew. Okay. See 985e86a. But that causes an error if file does not exist… I thought new goal was to not create files unless necessary.
E. To avoid error and directory and file content, create empty files. Proposal: I add back "CMS" section to
README.md
but with new command. See985e86a...3c0187bb
.I'm fine with [any…] just mentioning drawbacks.
—
P.S. [Devs have so many commands] to run… couldn't some be moved to a script that Docker runs on
up
?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.
Also, if no rush on this, it can stay open to remind me to do Idea D.
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.
Thanks for background again. Idea D is best imo