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

Unable to start two apps with the same name - When using variables and includes in server.xml to resolve app location #1145

Closed
scottkurz opened this issue May 21, 2021 · 6 comments · Fixed by #1655
Assignees

Comments

@scottkurz
Copy link
Member

scottkurz commented May 21, 2021

With liberty-maven-plugin 3.3.4, we hit the same symptom as in #583, see #583 (comment).

use case

In server.xml

<include location="${shared.config.dir}/environment.xml"/>

<webApplication location="${server.config.dir}/apps/${project.artifactId}.war" contextRoot="/" />

So I think there's two layers of failure here: first, we're failing to resolve ${shared.config.dir} in the include location. But even if we did, there'd be a second failure to resolve the ${server.config.dir} and substitution in the webApplication location when building the ServerConfigDocument to understand whether we need to generate the configDropin for app deployment.

Recreate

  1. git clone https://github.com/scottkurz/lmp-issue-587
  2. git checkout -b recreate a636095261d2f3097f7d39895cb8579bdf4d093a
  3. mvn resources:copy-resources liberty:dev

NOTES

There's a lot of overlap here with: #1039 but I'm not clear where that stands, if it's broader than resolving the app location or focused elsewhere. So let me open this new issue though also thinking a full solution

@cherylking
Copy link
Member

Looking at the code in ci.common, I don't think the pre-defined variables were ever handled for resolving variables in the application location attribute, but it should be handled for the include location attribute. The code in https://github.com/OpenLiberty/ci.common/blob/main/src/main/java/io/openliberty/tools/common/plugins/config/ServerConfigDocument.java is not looking for pre-defined Liberty variables. The code in https://github.com/OpenLiberty/ci.common/blob/main/src/main/java/io/openliberty/tools/common/plugins/util/ServerFeatureUtil.java does though. The same type of logic probably needs to be included in ServerConfigDocument.

@scottkurz
Copy link
Member Author

scottkurz commented May 26, 2021

Looking at the code in ci.common, I don't think the pre-defined variables were ever handled for resolving variables in the application location attribute,

And it's not clear there's any value in resolving ${server.config.dir} in the app location vs. using the relative location ('apps').. if that's harder or out of scope.

@cherylking
Copy link
Member

@scottkurz While working on the fixes in the two referenced PRs, I found I had to change one thing in the pom.xml (from what you had in the referenced repo) in the test case that I used to test the fix.

In your repo, you have this for copying the environment.xml file:

<outputDirectory>target/liberty/wlp/usr/shared/config</outputDirectory>

I had to change it to this in my test case:

<outputDirectory>target/liberty/usr/shared/config</outputDirectory>

It may just have to do with how the integration tests are configured. Also, I used the following steps in the test case instead of liberty:dev:

mvn clean liberty:create resources:copy-resources install

Matt has reviewed the PRs. Are you good with my merging them, or did you want to review them?

@scottkurz
Copy link
Member Author

@cherylking I'll take a look in the next couple days. Thanks.

@scottkurz
Copy link
Member Author

scottkurz commented Apr 3, 2023

My initial test of the original app seems to show the same high-level end result... we generate the install apps config dropin instead of using the one in the src server.xml. Will try to dig a bit further.

@scottkurz
Copy link
Member Author

Confirmed the PRs together seem to resolve my original recreate scenario on Windows.

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 a pull request may close this issue.

2 participants