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.

Sign up for GitHub

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

Make the names of the wikibase instance and WDQS adjustable in the settings #293

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Make the names of the wikibase instance and WDQS adjustable in the settings #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
995a92f
add MW_SITE_NAME as config parameter
DavidFichtmueller Jan 5, 2022
7717593
show wikibase name in Quickstatements
DavidFichtmueller Jan 5, 2022
f380436
make WDQS frontend name adjustable via .env file
DavidFichtmueller Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Docker/build/QuickStatements/config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"logfile" : "/var/log/quickstatements/tool.log" ,
"sites" : {
"${MW_SITE_NAME}" : {
"label" : "${MW_SITE_NAME}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really sure I understand this change, isn't $MW_SITE_NAME in this file a couple of times already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ${MW_SITE_NAME} in sites is the id for this particular configuration and the one in site: defines the default configuration. These two need to be the same, though it doesn't matter what they are. The could as well just be called "default". However the one in "label" is the one that is actually displayed in the user interface. See https://quickstatements.toolforge.org/config.json for another example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed another issue regarding this change with Quickstatements: When a name with capital letters (or leading/tailing white spaces) is used, QS has issues, because it handles the cleanup of the names different in different parts of the code. I made a PR about it: magnusmanske/quickstatements#30 . Until this is resolved we either might have to wait or add anothers sed replacement line in the QS Docker file: https://github.com/wmde/wikibase-release-pipeline/blob/main/Docker/build/QuickStatements/Dockerfile#L35

sed -i "s/$site = strtolower ( trim ( get_request ( 'site' , '' ) ) ) ;/$site = get_request ( 'site' , '' ) ;/g" /var/www/html/quickstatements/public_html/api.php

"oauth" : {
"language":"${MW_SITE_LANG}" ,
"project":"${MW_SITE_NAME}" ,
Expand Down
1 change: 0 additions & 1 deletion Docker/build/WDQS-frontend/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ COPY custom-config.json /templates/custom-config.json
COPY default.conf /templates/default.conf

ENV LANGUAGE=en\
BRAND_TITLE=DockerWikibaseQueryService\
COPYRIGHT_URL=undefined

ENTRYPOINT ["/entrypoint.sh"]
Expand Down
2 changes: 1 addition & 1 deletion Docker/build/WDQS-frontend/custom-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"urlShortener": "tinyurl"
},
"brand": {
"title": "$BRAND_TITLE",
"title": "${WDQS_NAME}",
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree $BRAND_TITLE isn't a great choice for the env var here, but would probably maybe require updating some documentation too if you search for $BRAND_TITLE in the repo it occurs a couple of times.

Copy link

Choose a reason for hiding this comment

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

I thought BRAND_TITLE was clear enough and find it clearer than WDQS_NAME. Note how the JSON calls it "brand title" as well, so there also is the consistency aspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah on a second glance I agree that consistency should have priority here, lets not do this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I changed this is to have all of the user adjustable settings in .env file start with WDQS_. While this brakes a bit of consistency within this template file, it increases the consistency for the user in the .env file, where they would have BRAND_TITLE next to WDQS_FRONTEND_HOST and WDQS_FRONTEND_PORT. Maybe this could be renamed to WDQS_FRONTEND_NAME though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point, maybe a compromise could be WDQS_FRONTEND_BRAND_TITLE, just rolls off the tongue ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Now that we found a compromise, the only piece of documentation that needs to be adjusted is the README.md. I can add another commit to this PR, but not before next week.

"logo": "logo.svg",
"favicon": "favicon.ico",
"copyrightUrl": "$COPYRIGHT_URL",
Expand Down
2 changes: 2 additions & 0 deletions example/docker-compose.extra.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ services:
environment:
- WIKIBASE_HOST=${WIKIBASE_HOST}
- WDQS_HOST=wdqs-proxy.svc
- WDQS_NAME=${WDQS_NAME}
wdqs:
image: "${WDQS_IMAGE_NAME}"
restart: unless-stopped
Expand Down Expand Up @@ -115,6 +116,7 @@ services:
- "WB_ITEM_PREFIX=Item:"
- OAUTH_CONSUMER_KEY=${OAUTH_CONSUMER_KEY}
- OAUTH_CONSUMER_SECRET=${OAUTH_CONSUMER_SECRET}
- MW_SITE_NAME=${MW_SITE_NAME}

volumes:
LocalSettings:
Expand Down
1 change: 1 addition & 0 deletions example/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ services:
<<: *wikibase_variables
WIKIBASE_PINGBACK:
MW_WG_ENABLE_UPLOADS:
MW_SITE_NAME: ${MW_SITE_NAME}

wikibase_jobrunner:
image: "${WIKIBASE_BUNDLE_IMAGE_NAME}"
Expand Down
2 changes: 2 additions & 0 deletions example/template.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ MW_ADMIN_NAME=admin
[email protected]
MW_SECRET_KEY=some-secret-key
MW_WG_ENABLE_UPLOADS=false
MW_SITE_NAME=wikibase-docker

## Jobrunner Configuration
MAX_JOBS=1
Expand All @@ -40,6 +41,7 @@ WIKIBASE_PORT=80
## WDQS-frontend Configuration
WDQS_FRONTEND_HOST=wdqs-frontend.svc
WDQS_FRONTEND_PORT=8834
WDQS_NAME=DockerWikibaseQueryService

## Quickstatements Configuration
# quickstatements.svc is the internal docker hostname, change this value to the public hostname
Expand Down