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.

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 GUI statement links #387

Merged
merged 6 commits into from
Oct 4, 2023
Merged

Fix GUI statement links #387

merged 6 commits into from
Oct 4, 2023

Conversation

eloiferrer
Copy link
Member

MaRDI Pull Request

Changes:

Instructions for PR review:

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

Checklist for this PR:

@eloiferrer eloiferrer temporarily deployed to staging August 11, 2023 15:12 — with GitHub Actions Inactive
@eloiferrer eloiferrer self-assigned this Aug 11, 2023
Copy link
Member

@physikerwelt physikerwelt left a comment

Choose a reason for hiding this comment

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

I think it would be better to overwrite the values for local development. If we merged this as it is it would break the staging software links.

@eloiferrer
Copy link
Member Author

eloiferrer commented Aug 14, 2023

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
Copy link
Member

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 eloiferrer temporarily deployed to staging September 4, 2023 11:49 — with GitHub Actions Inactive
Copy link
Member

@physikerwelt physikerwelt left a comment

Choose a reason for hiding this comment

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

I would have used getenv instead of _ENV but it does not really matter. Decide on your own feelings https://stackoverflow.com/a/8798318

@eloiferrer
Copy link
Member Author

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.

Copy link
Member

@physikerwelt physikerwelt left a comment

Choose a reason for hiding this comment

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

I think so.

@eloiferrer eloiferrer temporarily deployed to staging September 4, 2023 14:43 — with GitHub Actions Inactive
@eloiferrer eloiferrer temporarily deployed to staging September 4, 2023 14:56 — with GitHub Actions Inactive
@eloiferrer eloiferrer temporarily deployed to staging September 5, 2023 08:26 — with GitHub Actions Inactive
@eloiferrer
Copy link
Member Author

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

Copy link
Member

@physikerwelt physikerwelt left a comment

Choose a reason for hiding this comment

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

I think it is ok, however, I find maintaining the localhost:port alternative creates a huge overhead and makes the dev environment different from production so that some parts need to developed twice. I would thus recommend to remove the localhost:port alternative and use a pseudo domain that points to localhost instead.

@eloiferrer
Copy link
Member Author

eloiferrer commented Sep 5, 2023

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

WIKIBASE_HOST=localhost
. So no changes in the current dev environment are needed.

@physikerwelt
Copy link
Member

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.

- traefik.http.routers.service-quickstatements.rule=Host(`quickstatements.portal.mardi4nfdi.${TLD}`)
- traefik.http.routers.service-quickstatements.entrypoints=websecure
- traefik.http.routers.service-quickstatements.tls.certResolver=le
environment:
- QUICKSTATEMENTS_HOST=https://quickstatements.portal.mardi4nfdi.${TLD}
- WIKIBASE_SCHEME_AND_HOST=http://wikibase-docker.svc
- QS_PUBLIC_SCHEME_HOST_AND_PORT=https://quickstatements.portal.mardi4nfdi.${TLD}
- WB_PUBLIC_SCHEME_HOST_AND_PORT=https://portal.mardi4nfdi.${TLD}

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
Copy link
Member

@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.

@physikerwelt physikerwelt merged commit 63145cc into main Oct 4, 2023
3 checks passed
@physikerwelt physikerwelt deleted the fix_gui_statement_links branch October 4, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statements cannot be added locally through the GUI
2 participants