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

build: dockerize fuseki (dsp-30) #1653

Merged
merged 47 commits into from
Jun 26, 2020
Merged

build: dockerize fuseki (dsp-30) #1653

merged 47 commits into from
Jun 26, 2020

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Jun 10, 2020

  • add creating and publishing of own docker image: daschswiss/knora-fuseki
  • add running of tests with fuseki
  • remove GraphDB from Makefile
  • add repository create scripts (fuseki-init-knora-test.sh, etc.)
  • Upgrade integration test failing. Need to change scripts so they work with Fuseki
  • add automatic startup of containers when tests are running from from SBT.
  • SearchRouteV2R2RSpec

Postponed issues:

  • deleting the repository using the Fuseki Admin HTTP API does not work correctly. It needs to be deleted manually before running the fuseki-init-knora-test.sh script (DSP-432).

@subotic subotic self-assigned this Jun 10, 2020
@subotic subotic changed the title Wip/dsp 30 dockerize fuseki build: dockerize fuseki (dsp-30) Jun 10, 2020
@benjamingeer
Copy link

should not return duplicate results when there are unbound variables in one or more UNION
branches

I can look at this next week.

Upgrade integration test failing. Need to change scripts so they work with fuseki

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 git checkout fails. If there’s another error, let me know.

@benjamingeer
Copy link

OK I see, fuseki-empty-repository.sh and fuseki-upload-repository.sh (used only in the Makefile) are failing, maybe because Fuseki is running on a different port? Or not running in Tomcat?

@subotic
Copy link
Collaborator Author

subotic commented Jun 12, 2020

OK I see, fuseki-empty-repository.sh and fuseki-upload-repository.sh (used only in the Makefile) are failing, maybe because Fuseki is running on a different port? Or not running in Tomcat?

I think it should be fixed now. The URL is a bit different because Fuseki is not running in Tomcat.

@subotic
Copy link
Collaborator Author

subotic commented Jun 12, 2020

should not return duplicate results when there are unbound variables in one or more UNION
branches

I can look at this next week.

Thanks for taking a look. I don't really know what this test does.

@subotic
Copy link
Collaborator Author

subotic commented Jun 12, 2020

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.

@subotic
Copy link
Collaborator Author

subotic commented Jun 16, 2020

@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 make build-all-images before you run the tests from inside SBT so that the needed Docker images are built. You can, of course, also run make test if you like.

@subotic
Copy link
Collaborator Author

subotic commented Jun 23, 2020

@benjamingeer I'm struggling to get the sipi integration tests working with testcontainers, because there is no way to mount the shared tmp folder.

Could we maybe rewrite /v1/resources to use convert_from_file.lua instead convert_from_path.lua? Or is there something I'm missing? I vaguely remember that this was the plan at some point?

@benjamingeer
Copy link

I think what you want is #1233, which is blocked by dasch-swiss/sipi#309.

@subotic
Copy link
Collaborator Author

subotic commented Jun 23, 2020

I think what you want is #1233, which is blocked by dasch-swiss/sipi#309.

Yes, that's what I meant. Thanks!

@subotic subotic marked this pull request as ready for review June 25, 2020 15:21
@subotic subotic added the enhancement improve existing code or new feature label Jun 25, 2020
@subotic
Copy link
Collaborator Author

subotic commented Jun 26, 2020

@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

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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.

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?

Copy link
Collaborator Author

@subotic subotic Jun 26, 2020

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.

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?

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.

Copy link
Collaborator Author

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.

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?

Copy link
Collaborator Author

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.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

| | _ \/ ___|| _ \ / \ | _ \_ _|
| | | | \___ \| |_) |____ / _ \ | |_) | |
| | |_| |___) | __/_____/ ___ \| __/| |
| |____/|____/|_| /_/ \_\_| |___|

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

Copy link
Collaborator Author

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.

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?

Copy link
Collaborator Author

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.

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator Author

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.

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?

Copy link
Collaborator Author

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.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link

@benjamingeer benjamingeer left a 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!

@subotic
Copy link
Collaborator Author

subotic commented Jun 26, 2020

Me too. Thanks for making it possible in the first place :-)

@subotic
Copy link
Collaborator Author

subotic commented Jun 26, 2020

And thanks for the review :-)

@subotic subotic merged commit e4fd02c into develop Jun 26, 2020
@subotic subotic deleted the wip/DSP-30-dockerize-fuseki branch June 26, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants