-
Notifications
You must be signed in to change notification settings - Fork 18
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
build: dockerize fuseki (dsp-30) #1653
Conversation
I can look at this next week.
There are no scripts to change, everything is done in Knora, including support for upgrading a repository in Fuseki. It looks like the tests are failing in GitHub CI because |
OK I see, |
I think it should be fixed now. The URL is a bit different because Fuseki is not running in Tomcat. |
Thanks for taking a look. I don't really know what this test does. |
Maybe we can talk on Monday. There are some limitations because removing a repository doesn't work over the Fuseki Admin HTTP API. The current Makefile plumbing is good enough for CI and for a front-end developer or research-IT that need to start the whole stack. But for a DSP-API developer it is not a nice experience. We should discuss how it should work for the developer. @SepidehAlassi mentioned https://www.testcontainers.org. By using this, the starting of the DB, Redis, and SIPI containers would happen automatically at the beginning of each test. As a developer, you would only need to start the tests, everything else would happen automagically. |
@benjamingeer OK, you can work now on the one failing test. There is currently a problem with Github-CI. To run the tests locally, you need to run |
@benjamingeer I'm struggling to get the sipi integration tests working with Could we maybe rewrite |
I think what you want is #1233, which is blocked by dasch-swiss/sipi#309. |
Yes, that's what I meant. Thanks! |
…knora-api into wip/DSP-30-dockerize-fuseki
@benjamingeer It would be great if you could do a quick review. I would like to first merge this PR before I start working on the Sipi v3.0.0 PR. |
README.md
Outdated
|
||
First, open an [issue](https://github.com/dhlab-basel/Knora/issues) to describe your problem or idea. We may ask you to submit a [pull request](https://help.github.com/articles/about-pull-requests/) implementing the desired functionality. | ||
First, open an [issue](https://github.com/dhlab-basel/Knora/issues) to describe |
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.
Should be https://github.com/dasch-swiss/knora-api/issues now.
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.
Thanks, fixed. I have found a few more old links and fixed those also.
fi | ||
} | ||
|
||
# delete-repository // delete dos not work correctly. need to delete database manually. |
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.
Why not send a SPARQL DROP ALL
instead of deleting the repository?
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.
Init is used to initialize the repository, i.e., create a new repository based on the configuration which can change. If we only empty the repository, then a new one cannot be created, because there is already one with that name.
The Admin API route for deleting the repository only deletes the TDB2 database but does not delete the Apache Lucene database, so when a new repository is created, an error message is returned. This is why the repository needs to be deleted by hand, or in our case through a special make target.
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.
Could we just ignore those errors?
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.
Never mind, I understand: you need to be able to recreate the repository with a different configuration.
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.
👍
* Send a GET request to Sipi, asking for the preview image. | ||
* With testcontainers it is not possible to know the random port | ||
* in advance, so the started sipi container cannot be configured | ||
* correctly. |
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 don't understand. Does this mean that this test can't work with testcontainers? Does something need to be fixed here?
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 have a workaround for this. In the tests, I rewrite the returned standard URL from Sipi to the correct one. We are probably overspecifying things in Sipi, which makes the whole thing a bit unflexible.
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.
Could you clarify the comment? "the started sipi container cannot be configured correctly" makes it seem as if something is broken and needs to be fixed.
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.
done
| | _ \/ ___|| _ \ / \ | _ \_ _| | ||
| | | | \___ \| |_) |____ / _ \ | |_) | | | ||
| | |_| |___) | __/_____/ ___ \| __/| | | ||
| |____/|____/|_| /_/ \_\_| |___| |
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'm confused about these names. I thought "Knora" and "DSP-API" were the same thing. Personally I think "Knora" is a better name than "DSP-API" (takes too long to say and is impossible to remember).
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.
Knora is slowly phased out. The official name of the platform is DSP, hence DSP-API.
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 think DSP is a terrible name. Do a Google search for "DSP" and see what you get. Can the whole team have a discussion about this?
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.
Sure, we can do it next Tuesday after the Daily Scrum.
@@ -607,6 +600,65 @@ class HttpTriplestoreConnector extends Actor with ActorLogging with Instrumentat | |||
} | |||
} | |||
|
|||
/** | |||
* Initialize the Jena Fuseki triplestore. Currently only works for 'knora-test' | |||
* and 'knora-test-unit' repository names. |
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.
Why?
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.
because here I'm using the two Fuseki configs that we have (knora-test and knora-test-unit). I guess I can make it configurable, but then maybe in another PR. I've created https://dasch.myjetbrains.com/youtrack/issue/DSP-434.
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'm just trying to understand the use case for this method. Does this method only need to be called for knora-test
and knora-test-unit
, or is this a limitation that needs to be dealt with?
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 method, in my opinion, mainly makes sense for automated testing, to make life easier. To be used, the API needs to be started with a certain flag, which I personally would never use or recommend for production.
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.
OK, then please just clarify that in the comment.
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.
done
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.
OK, this is great, I'm really happy to see it finally working! Thank you!
Me too. Thanks for making it possible in the first place :-) |
And thanks for the review :-) |
daschswiss/knora-fuseki
fuseki-init-knora-test.sh
, etc.)Postponed issues:
fuseki-init-knora-test.sh
script (DSP-432).