-
Notifications
You must be signed in to change notification settings - Fork 10
Conversation
@Saphyel makes sense - could you refactor this PR just to include the module? Did you test that the CDN module works correctly? Maybe write a couple of lines on the way to configure this? |
…n into feature/PLAT-177_cdn
@Saphyel made a couple of updates here... its disabled in the profile and ive updated a comment from the install file. Ill assign this back to you to finish off the testing and implementation of our cdn settings. |
@bimsonz why you disabled it from the profile? |
@Saphyel on your suggestion that we split this into two prs.. we dont need this enabled just yet and will slow down the build, if you finish it in this pr you can re enable it |
* Uses hook_install(). | ||
*/ | ||
function cr_cdn_install() { | ||
Drupal::configFactory()->getEditable('cdn.settings')->set('status', 1)->save(TRUE); |
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.
@Saphyel we'll be using CMI proper (using a config folder) for RND17 (see e.g. https://github.com/comicrelief/rnd17/tree/develop/sites/default/config) so this config change will live there and we don't need to do crazy things like editing config in install hooks.
As a result, I think we can get entirely rid of cr_cdn as a module, it'll just live as config in the sites/default/config
dir for RND17
\cc @Saphyel
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.
Well in that case we configure CDN on the fly·
@pvhee care to merge? |
Fixes https://jira.comicrelief.com/browse/PLAT-177
Changes proposed in this pull request
This is actually block, because we need to build the cdn server (webops!)... maybe we can split into 2 this PR, one just for enable the module and other for configure it