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 jetty-eeX-env.xml, as per jetty-eeX-web.xml #11752

Merged
merged 5 commits into from
May 10, 2024

Conversation

janbartel
Copy link
Contributor

Need to support ee specific jetty-env.xml file, just as we do for jetty-web.xml

@janbartel janbartel requested a review from gregw May 6, 2024 05:19
@janbartel janbartel self-assigned this May 6, 2024
@@ -47,6 +47,8 @@ public class EnvConfiguration extends AbstractConfiguration
private static final Logger LOG = LoggerFactory.getLogger(EnvConfiguration.class);

private static final String JETTY_ENV_BINDINGS = "org.eclipse.jetty.jndi.EnvConfiguration";
Copy link
Contributor

Choose a reason for hiding this comment

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

this binding key isn't ee10 specific, should this be in core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe but there's no good place to put it (yet?).

@@ -47,6 +47,8 @@ public class EnvConfiguration extends AbstractConfiguration
private static final Logger LOG = LoggerFactory.getLogger(EnvConfiguration.class);

private static final String JETTY_ENV_BINDINGS = "org.eclipse.jetty.jndi.EnvConfiguration";
Copy link
Contributor

Choose a reason for hiding this comment

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

this name isn't ee11 specific, shouldn't this be in jetty-core?

@@ -50,6 +49,8 @@ public class EnvConfiguration extends AbstractConfiguration
private static final Logger LOG = LoggerFactory.getLogger(EnvConfiguration.class);

private static final String JETTY_ENV_BINDINGS = "org.eclipse.jetty.jndi.EnvConfiguration";
Copy link
Contributor

Choose a reason for hiding this comment

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

This name isn't ee9 specific, shouldn't it be in jetty core?

@janbartel janbartel requested a review from joakime May 9, 2024 07:53
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Minor nit... I found the xmlFile field confusing and was looking for more behaviour for it. I don't think we need it in the debug as the exception itself will be clear

janbartel and others added 3 commits May 10, 2024 08:38
…0/plus/webapp/EnvConfiguration.java

Co-authored-by: Greg Wilkins <gregw@webtide.com>
…lus/webapp/EnvConfiguration.java

Co-authored-by: Greg Wilkins <gregw@webtide.com>
…1/plus/webapp/EnvConfiguration.java

Co-authored-by: Greg Wilkins <gregw@webtide.com>
@gregw
Copy link
Contributor

gregw commented May 10, 2024

I think this and the jetty-web.xml change should be backported to 12.0

@janbartel janbartel merged commit 1e0f012 into jetty-12.1.x May 10, 2024
1 of 4 checks passed
@janbartel janbartel deleted the jetty-12.1.x-jetty-ee-env-xml branch May 10, 2024 06:43
janbartel added a commit that referenced this pull request Jun 26, 2024
* Support jetty-eeX-env.xml, as per jetty-eeX-web.xml

---------

Co-authored-by: Greg Wilkins <gregw@webtide.com>
@olamy olamy mentioned this pull request Oct 17, 2024
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.

3 participants