-
Notifications
You must be signed in to change notification settings - Fork 3
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
[110] Upgrade Weaver-Webservice-Core (2.1.1-RC8). #111
Conversation
kaladay
commented
Apr 27, 2022
•
edited
Loading
edited
- Upgrade to Java 11.
- Upgrade all dependencies with vulnerabilities.
- Upgrade tests to use Junit5.
- Add docker file.
- Add comment about why remaining deprecated code is not replaced.
- Upgrade to Java 11. - Upgrade all dependencies with vulnerabilities. - Upgrade tests to use Junit5.
Failure message: ``` Error: Failed to execute goal org.codehaus.mojo:cobertura-maven-plugin:2.7:instrument (default-cli) on project ir-iiif-service: Execution default-cli of goal org.codehaus.mojo:cobertura-maven-plugin:2.7:instrument failed: Plugin org.codehaus.mojo:cobertura-maven-plugin:2.7 or one of its dependencies could not be resolved: Could not find artifact com.sun:tools:jar:0 at specified path /opt/hostedtoolcache/jdk/11.0.15/x64/../lib/tools.jar -> [Help 1] ``` The tests worked previously prior to adding a version number. Remove the version number and hope it does not fail.
Cobertura doesn't work under JDK11 and is not well maintained anymore. Update Coveralls to work under JDK11. This required bringing in javax.xml.bind:jaxb-api within the plugin of coveralls.
It seems that the group id cannot be passed as a parameter because the `COPY --chown=` command does not support that. Use a relatively high number, such as 3001 for the uid and gid to reduce potential id conflicts with the system loaded by docker. Use a single variable ARG declaration for each variable and then import them into each stage. This must be done because ARG is lost between stages and we really should avoid exposing these via ENV. Use a custom work directory owned by a created user. Run maven as that user to avoid doing anything as root (security improvement). Run the service as that user from within that directory. The jar is moved and renamed for convenience. Maven (and possibly java) can be picky about arguments, so use the bracket notation for passing arguments to both maven and java.
This was being experimented with and was not intended to be committed.
…e changes. Move the hardcoded 3001 into USER_ID argument. Use `apt-get` instead of `apt`, they are apparently different programs. It turns out the user does have to be recreated across stages. Docker was subtly failing and prenenting to perform all of the other steps. Only the following message is evidence of the problem. ``` linux spec user: unable to find user iriifservice: no matching entries in passwd file ```
This is done either adding repackage to the pom.xml or by adding to the command line, like this: ``` mvn clean package spring-boot:repackage -Pjar -DskipTests=true ``` This is spring and it should be transparent, so add it to the pom.i
Change it to RUN until such time CMD can be figured out.
…start and not build. This is why nothing happens. Revert the change and put CMD back.
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.
Looks good. Will give us upgraded dependencies, improved test logging, and more secure docker build. I advocate for this docker approach be our pattern to follow.
@jeremythuff it would be nice to have your sign off before we copy this docker build pattern. |
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.
Looks great!
Move `file:` yaml property up to be alphabetical in ordering. Increase verbosity of logging to `WARN`. We probably want to see any warnings when running tests as they may matter.
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.
The only change I would suggest is to remove the as runtime
from line 43 since it's not used.
The `as runtime` is not being used.
84ef278
…blems. Update the SOURCE_DIR as it doesn't need the leading slash. The copy over build artifact in the maven stage is not used. Remove it.
Do this for consistency reasons.