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

Support for multiple war files #402

Merged
merged 7 commits into from
Jan 31, 2024

Conversation

engine-workflow
Copy link

Summary

I am trying to solve the issue#134 so that multiple war files can be deployed.

Use Cases

  1. if only one war file, same as previous design.
  2. if two or more war files in the application path /workspace, use /workspace as the webapps/ and explode war files into folders with same war file's name without file extension.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@engine-workflow engine-workflow requested a review from a team as a code owner November 2, 2023 21:30
Copy link

linux-foundation-easycla bot commented Nov 2, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmikusa dmikusa added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Nov 15, 2023
tomcat/detect.go Outdated Show resolved Hide resolved
tomcat/detect.go Outdated Show resolved Hide resolved
tomcat/build.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

I tested this but had issues with detection.

My test had a directory with two WAR files in it. I did a pack build -p ./dir-with-warfiles and had issues with detection. The Tomcat buildpack output:

PASSED: a WEB-INF directory was not found, this is normal when building from source

So I think it is going down the path of doing a build from source code, but this isn't a source code build.

What are the test cases that you tried? Were you building from source? If you have a project that produces multiple WAR files and could share it, that would help with testing.

There are five scenarios that come up in my head that we need to make sure work here:

  1. Build from source, produces a single WAR
  2. Build from source, produces multiple WAR files
  3. Precompiled assets single WAR with path pointing to the WAR, this is presented to the buildpacks as an exploded WAR directory
  4. Precompiled assets path pointing to a directory with multiple WAR files in it, buildpacks see the same
  5. Precompiled assets path pointing to a directory with a single WAR file in it, buildpacks will see the WAR file not an exploded WAR.

Let me know if you have questions about those scenarios.

@Weirong-Zhu
Copy link
Contributor

Weirong-Zhu commented Nov 21, 2023

I tested this but had issues with detection.

My test had a directory with two WAR files in it. I did a pack build -p ./dir-with-warfiles and had issues with detection. The Tomcat buildpack output:

PASSED: a WEB-INF directory was not found, this is normal when building from source

So I think it is going down the path of doing a build from source code, but this isn't a source code build.

What are the test cases that you tried? Were you building from source? If you have a project that produces multiple WAR files and could share it, that would help with testing.

There are five scenarios that come up in my head that we need to make sure work here:

  1. Build from source, produces a single WAR
  2. Build from source, produces multiple WAR files
  3. Precompiled assets single WAR with path pointing to the WAR, this is presented to the buildpacks as an exploded WAR directory
  4. Precompiled assets path pointing to a directory with multiple WAR files in it, buildpacks see the same
  5. Precompiled assets path pointing to a directory with a single WAR file in it, buildpacks will see the WAR file not an exploded WAR.

Let me know if you have questions about those scenarios.

@dmikusa, thank you so much for providing above testing cases. I am working on a testing repository. I will let you know when it is done.

@Weirong-Zhu
Copy link
Contributor

I tested this but had issues with detection.

My test had a directory with two WAR files in it. I did a pack build -p ./dir-with-warfiles and had issues with detection. The Tomcat buildpack output:

PASSED: a WEB-INF directory was not found, this is normal when building from source

So I think it is going down the path of doing a build from source code, but this isn't a source code build.

What are the test cases that you tried? Were you building from source? If you have a project that produces multiple WAR files and could share it, that would help with testing.

There are five scenarios that come up in my head that we need to make sure work here:

  1. Build from source, produces a single WAR
  2. Build from source, produces multiple WAR files
  3. Precompiled assets single WAR with path pointing to the WAR, this is presented to the buildpacks as an exploded WAR directory
  4. Precompiled assets path pointing to a directory with multiple WAR files in it, buildpacks see the same
  5. Precompiled assets path pointing to a directory with a single WAR file in it, buildpacks will see the WAR file not an exploded WAR.

Let me know if you have questions about those scenarios.
@dmikusa, here is the testing repo: https://github.com/Weirong-Zhu/cnb-tomcat-example. Environment variables I used for build are set in buildpack/*.env files.

@Weirong-Zhu
Copy link
Contributor

@dmikusa, could you please review the code change and test the sample repo?
https://github.com/Weirong-Zhu/cnb-tomcat-example.

@dmikusa
Copy link
Contributor

dmikusa commented Dec 2, 2023

Apologies. You're on my list of PRs to review, but it's been busy lately. I promise I'll get to this as soon as I can.

@dmikusa
Copy link
Contributor

dmikusa commented Jan 29, 2024

Again, sorry for taking so long to review this.

I did some testing:

  1. Build from source, produces a single WAR ✅
  2. Build from source, produces multiple WAR files ✅
  3. Precompiled assets single WAR with path pointing to the WAR, this is presented to the buildpacks as an exploded WAR directory ✅
  4. Precompiled assets path pointing to a directory with multiple WAR files in it, buildpacks see the same 💥
  5. Precompiled assets path pointing to a directory with a single WAR file in it, buildpacks will see the WAR file not an exploded WAR. 💥

The last two situations fail because detection criteria do not take this situation into consideration, so you'll see it output:

PASSED: a WEB-INF directory was not found, this is normal when building from source

and then fail cause it can't put together a buildplan. If the buildpack gets a directory of WAR files (precompiled), then it needs to handle this as well.

How would you like to proceed here? You can either push an update to this PR to address the last two cases, or I can merge what we have and I can follow up with a PR to make these last two cases work. I'm OK merging what you have, if you want to go that route, because the existing cases continue to work, and this is adding support for one of the new cases. Let me know what you'd prefer.

Thanks

@Weirong-Zhu
Copy link
Contributor

Weirong-Zhu commented Jan 29, 2024

Again, sorry for taking so long to review this.

I did some testing:

  1. Build from source, produces a single WAR ✅
  2. Build from source, produces multiple WAR files ✅
  3. Precompiled assets single WAR with path pointing to the WAR, this is presented to the buildpacks as an exploded WAR directory ✅
  4. Precompiled assets path pointing to a directory with multiple WAR files in it, buildpacks see the same 💥
  5. Precompiled assets path pointing to a directory with a single WAR file in it, buildpacks will see the WAR file not an exploded WAR. 💥

The last two situations fail because detection criteria do not take this situation into consideration, so you'll see it output:

PASSED: a WEB-INF directory was not found, this is normal when building from source

and then fail cause it can't put together a buildplan. If the buildpack gets a directory of WAR files (precompiled), then it needs to handle this as well.

How would you like to proceed here? You can either push an update to this PR to address the last two cases, or I can merge what we have and I can follow up with a PR to make these last two cases work. I'm OK merging what you have, if you want to go that route, because the existing cases continue to work, and this is adding support for one of the new cases. Let me know what you'd prefer.

Thanks

@dmikusa, thanks a lot for reviewing this PR. I would like merging it first, since I do not know how to reproduce the last two cases. So, could you please give me some instructions to tackle these two cases? By the way, do you mind if I directly contact you in slack?

@dmikusa
Copy link
Contributor

dmikusa commented Jan 31, 2024

I would like merging it first, since I do not know how to reproduce the last two cases.

👍

So, could you please give me some instructions to tackle these two cases?

1.mkdir single-war
2. ./mvnw package one of your apps or a Paketo sample app
3. Move that into the single-war/ directory
4. pack build -p ./single-war/ and include all the other options for pack build you'd normally use

Pack will use the directory as the application contents, so inside the container buildpacks will see the contents of that folder as /workspace.

1.mkdir multi-war
2. ./mvnw package one of your apps or a Paketo sample app
3. Copy that into the multi-war/ directory twice with different names.
4. pack build -p ./multi-war/ and include all the other options for pack build you'd normally use

Pack will use the directory as the application contents, so inside the container buildpacks will see the contents of that folder as /workspace.

By the way, do you mind if I directly contact you in slack?

Not at all. Feel free to reach on in the Paketo slack. I'm usually around there.

@dmikusa dmikusa merged commit 82c82a4 into paketo-buildpacks:main Jan 31, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants