MaRDI4NFDI / portal-compose

docker-composer repo for mardi

https://portal.mardi4nfdi.de

GNU General Public License v3.0

3 stars 1 forks source link

Fix GUI statement links #387

Closed eloiferrer closed 9 months ago

eloiferrer commented 11 months ago

MaRDI Pull Request

Changes:

This is an attempt to fix https://github.com/MaRDI4NFDI/portal-compose/issues/386 by changing the paths.
This also fixes a couple of mistakes regarding the env variables in docker-compose.yml

Instructions for PR review:

[ ] Conceptual Review (Logic etc...)
[ ] Code Review (Review your implementation)
[ ] Checkout (Test changes locally)

Checklist for this PR:

eloiferrer commented 11 months ago

Isn't there some variable that saves the portal.mardi4nfdi.de url and that can be accessed from Wikibase.php? It somehow doesn't look right to have this hard-coded urls in there.

physikerwelt commented 11 months ago

I think this is the variable to be set. Maybe there is something like concept URI from the linkedwiki extension, but I don't know.

eloiferrer commented 10 months ago

Ok. Since we will have now a PORTAL_HOST variable with portal.mardi4nfdi.de as value, would it be ok if I change all the instances of portal.mardi4nfdi.${TLD} in docker-compose.yml to just ${PORTAL_HOST}? e.g. quickstatements.portal.mardi4nfdi.${TLD} to quickstatements.${PORTAL_HOST} and so on.

eloiferrer commented 10 months ago

@physikerwelt I changed the logic again, so maybe just take a final look, before I merge it.

eloiferrer commented 10 months ago

I don't see if that is really the case here. This implementation does not change the current workflow we have. We are already changing the WIKIBASE_HOST and PORT for development https://github.com/MaRDI4NFDI/portal-compose/blob/f5ef817eaebb8aeda59447c23f8051ee77608a8c/README.md?plain=1#L21. So no changes in the current dev environment are needed.

physikerwelt commented 10 months ago

I don't see if that is really the case here. This implementation does not change the current workflow we have.

Yes, I fully agree. So it is as bad as it was before:-)

We are already changing the WIKIBASE_HOST and PORT for development

At least when trying to get quick statements to run, I found the combination of hosts and ports quite confusing.

https://github.com/MaRDI4NFDI/portal-compose/blob/f5ef817eaebb8aeda59447c23f8051ee77608a8c/docker-compose.yml#L321-L328

For example, https://quickstatements.portal.mardi4nfdi.${TLD} points to nowhere in local dev.

This PR, in particular,

https://github.com/MaRDI4NFDI/portal-compose/pull/387/files#diff-1593a24ff7c433502c819e45226b229d3c70082067e354b7208944164926a3f0R9-R13

adds further technical debts to handle both cases, dev, and prod. Instead, I suggest it would be easier to guide developers to install a custom DNS like described here https://github.com/bnfinet/docker-dns

physikerwelt commented 9 months ago

@eloiferrer what do you think?

I checked https://staging.swmath.org/wiki/Mathoid and saw that the link to the MaRDI portal item is currently missing, but the other direction from https://portal.mardi4nfdi.de/wiki/Item:Q38023 works. So that needs to be fixed anyhow. So I think it would be a good time to merge this now.