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
Starter site integration #226
Starter site integration #226
Changes from 9 commits
10fb73a
d59f15e
a97bb9a
c420c2b
7b8ce49
d16d818
500e8d1
8f978c2
2137655
a18bde0
52f962a
dd55067
d27c07c
785491b
11cdd49
80e4be7
61bb5ac
3eba24e
632f2af
54ee90d
b3f1bca
2b26252
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.
Not sure what the best approach was here, to allow for more granular control over what was being done in what was originally called the "Trusted Host Settings" bit, which was doing more than just setting the "trusted hosts" settings... elected to keep the original, but additionally have all the bits it contained be available in separate chunks, as we don't really want the
content/sync
thing being done in thestarter
"profile".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.
Looking back at https://github.com/Islandora-Devops/islandora-playbook/pull/126/files#diff-5bddb401e18178b7584b51cd9831eaa348385079b2224a85e3db8fa2412fc547L5-R15 ... I believe this is the intent, as otherwise, it seems that the
set_search_api_config
bit would've been being registered by the "Matomo" setup, in thewebserver-app/tasks/drupal.yml
bit?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.
Yeah that seems like a typo in the Matomo stuff.
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 should eventually be pointed at whatever reference structure we move to use for releases... or maybe left without a version reference to pull whatever "latest" version?
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.
Some of these - setting up Matomo and FITS - seem like they're
false
only because theyre out of scope at the moment but we probably eventually want them. Can you denote/comment/group them in some way to distinguish from the others which we do/don't need due to the architecture of the Starter Site?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.
Took a shot in 52f962a ... 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.
I feel like I should mention: Anything that does need to do any configuration probably maybe shouldn't do it by
config:set
'ing anything, but instead to use other mechanisms such asconfig_override
to avoid potentially dirtying configs... to avoid specific environmental concerns creeping into things; for example: If thematomo.settings:url_http
value was made to dynamically point at the hostname instead of just specifyinglocalhost:8000
, then yeah, it wouldn't really belong in the config that gets used between environments where that remote would(/could) vary.... This... does kind of get into a weird area with how the JWT stuff is presently configured, in that because things were based on the "standard" playbook install, that it just so happens to match the paths; however, if other paths are ever specified for
webserver_app_jwt_key_path
, then it could end up diverging...