-
Notifications
You must be signed in to change notification settings - Fork 477
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
9434 app container for dev #9439
Conversation
@poikilotherm this is working great! Thanks for the heads up about email. I tried using the "Support" link in the header but when I clicked "Send Message" I got this error:
Honestly, I can live without email working but it would be nice to see if we can fix this. I can also live without the TODO above about limiting resources (for now). Future PR, I'd say. I marked it as optional. I added docs and a release note. I tried to indicate that this is dev only. Hopefully the last thing is the SMTP thing. Or, again, I'm fine with fixing later. There is a TON of value in this PR. Thanks!! |
Here's a stacktrace of the MailDev/SMTP problem: stacktrace.txt Apparently, fromAddress here is null:
In the domain.xml, the from address is
The from address is not defined as a database setting:
Setting it might fix it. If I set :SystemEmail like this...
I get a new error:
From my Mac, the SMTP server is running on port 25...
... but the dataverse-1 (app container) can't reach it:
Why? |
I have done the following steps so far:
Received this error within the docker-compose logs while applying the third step:
Dataverse runs on 8080, but when trying to Log In with dataverseAdmin / admin credentials, the authentication fails. Am I missing something? |
@GPortas please note that the login is "dataverseAdmin:admin1". That "1" is easy to forget. Also, the "getSingleResult()" thing is very annoying but harmless. For some reason no one ever fixed this also it would be a low hanging fruit. Same goes for all the SQL exceptions which are triggered by us generating the DDL everytime we start the application. |
All containers have static names now, which might be a problem down the road. But at least consistent now.
The approach is also missing R and Rserve. They are listed as a prerequisite within the documentation but it looks like they are not really needed. If R/RServe is optional the documentation must be updated, otherwise another container wrapping RServe is needed. https://guides.dataverse.org/en/latest/installation/prerequisites.html?highlight=rserve |
Thanks @johannes-darms for the extensive review of the PR! I added comments to your comments.
Two things here: 1) Rserve is not a necessary component for developing Dataverse, which is what this is about. 2) Technically it even is not a requirement for an installation if you don't do any ingest. But yes, that should be stated in the docs. Would you be able to create an issue and/or a pull request? |
Good point. @poikilotherm how do you feel about a new title for this PR? "app container" -> "app container for dev" @johannes-darms thanks for all the feedback! I also left comments on your comments. Much appreciated!! |
Although the asadmin commands are enough to manage the Payara server, sometimes it is very useful to access the Payara administration console for better visualization. By default, Payara exposes the administration console on port 4848. It would be interesting to map this port to outside the container within the docker-compose file. I've tested this with this branch, by adding The problem is that the admin console now expects user and password credentials to login (As far as I know, these credentials are not documented). With the normal installation (without containers), we don't need credentials for accessing the Payara admin console. Is that login requirement a configuration that comes from the base image? It would be great to make admin credentials configurable through container environment variables. |
@GPortas yes, this comes from the base image. We could add a section to the base image documentation, similar to upstream Payara as this works exactly the same in our custom base image. In fact, the credentials are available via the (currently undocumented) env vars @GPortas probably this is beyond scope for this PR. Would you add a note to https://docs.google.com/document/d/15-sqdKzpCgQBtaPaAGYMaqcxsQXmt-2CwAVNPmmAPRY so we don't forget? |
@pdurbin @johannes-darms I added a few pieces of information about a future sunset of docker-aio in 7bcadab. Let me know if that helps. Feel free to hack on it. |
Related: |
Issues so far:
|
Hi @kcondon, thx for trying this. The log says that the database connection could not be established. It looks like you already had the local volumes from before (most likely #9414 / #9417), so there might be a different database already in place. Could you delete the whole |
Without setting the "env var" for docker compose within the Maven POM, the value will not be set, leading to a broken DB connection. The DMP does not read the values from .env.
Woo-hoo thanks for merging @kcondon ! Much appreciated, as always!! |
What this PR does / why we need it:
This pull request will add capabilities to create a Dataverse application image and run it along all necessary dependencies.
Which issue(s) this PR closes:
Special notes for your reviewer:
There are some TODOs:
(Optional) Maybe add some more resource limitations. Maybe tweak Dataverse container to fit into less than 2 GiB of RAMSuggestions on how to test this:
Clone/switch. Ensure to have Docker running. Execute
mvn -Pct clean package docker:run
, then bootstrap when startup is done with./scripts/dev/docker-final-setup.sh
and go to localhost:8080.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
Yes, included.
Additional documentation:
None yet.