Skip to content

Issue 52855: Missing Servlet API when extracting remote pipeline resources #1045

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

Merged
merged 2 commits into from
Apr 17, 2025

Conversation

labkey-jeckels
Copy link
Contributor

Rationale

We have two places that extract JARs for remote pipeline scenarios. This one, used in our tests, and another used from the command line. They should be kept in sync.

Related Pull Requests

Changes

  • Pointer to the other spot
  • Stronger error checks
  • Update to grab the new home for the servlet API classes

Copy link
Contributor

@labkey-adam labkey-adam left a comment

Choose a reason for hiding this comment

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

I don't suppose we have a test that alerts us to issue like this...

@bbimber
Copy link
Contributor

bbimber commented Apr 16, 2025

Hi @labkey-jeckels, I am looking at ways to add testing over this. If labkeyServer.jar exists, then it seems like it would be easy to have an integration that that either: 1) executes the extract command and verifies the results, or 2) maybe refactor this code to separate a method that inspects the JAR and returns a list of files to extract from the code that actually does the extraction. The former could be tested without actually extracting files.

However, unless someone actually runs the a gradle dist step, does labkeyServer.jar exist on a normal TeamCity and/or dev machine? It's possible I havent fully wrapped my head about all the uses here.

Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

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

Just some error message changes

@labkey-jeckels
Copy link
Contributor Author

Hi @labkey-jeckels, I am looking at ways to add testing over this. If labkeyServer.jar exists, then it seems like it would be easy to have an integration that that either: 1) executes the extract command and verifies the results, or 2) maybe refactor this code to separate a method that inspects the JAR and returns a list of files to extract from the code that actually does the extraction. The former could be tested without actually extracting files.

However, unless someone actually runs the a gradle dist step, does labkeyServer.jar exist on a normal TeamCity and/or dev machine? It's possible I havent fully wrapped my head about all the uses here.

It should exist on a TeamCity agent. On dev machines we have build/deploy/embedded/embedded-25.5-SNAPSHOT.jar which is a precursor to labkeyServer.jar which has a subset of files. The full JAR is bigger and slower to build than we want to do on a typical dev machine build.

I'm also pondering how to get better coverage on this, and whether it would give meaningful coverage or not. I mistakenly thought we had reasonable coverage from the PipelineServiceImpl variant here, but obviously it wasn't close enough. The other challenge is the class loader boundaries between the embedded/bootstrap code and the rest of the app.

@bbimber
Copy link
Contributor

bbimber commented Apr 17, 2025

hi @labkey-jeckels: OK. What do you think about merging this PR to restore functionality, and then address testing / future-proofing of this in another PR? I have not had time to really dig into this myself, but would be willing to give it a try. I also did not really look in depth that the two different code paths that exist (PipelineServiceImpl and this), but obviously it would be nice if they were more formally synchronized.

My github-actions CI builds a distribution, and it would be relatively easy for this code to run '-extract' and inspect the results; however, i think it's better to put something into core labkey.

@labkey-jeckels labkey-jeckels merged commit f7fe8d4 into release25.3-SNAPSHOT Apr 17, 2025
5 checks passed
@labkey-jeckels labkey-jeckels deleted the 25.3_fb_52855_servletApi branch April 17, 2025 17:40
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.

4 participants