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

PAYARA-3718 Adds EJB over HTTP client config via system properties #3930

Merged

Conversation

@jbee
Copy link
Contributor

commented May 3, 2019

Description

I'm not entirely sure this is what we want but I think this is what we tried to do. I figured it is easiest to do the fallback in the factory. All environment variables of javax.naming.Context and our RemoteEJBContextFactory are checked for being defined as system property of the same name.

@jbee jbee added this to the 5.192 milestone May 3, 2019

@jbee jbee self-assigned this May 3, 2019

@jbee jbee requested review from pdudits and Pandrex247 May 3, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

jenkins test please

@smillidge
Copy link
Contributor

left a comment

Not clear from the code here but are the standard JNDI system properties supported i.e.

java.naming.security.principal java.naming.security.credentials java.naming.factory.initial java.naming.provider.url

Customer wanted to support the standard system properties so that the environment properties don't need to be set.

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@smillidge there is the added array of properties checked. If we want to support all of the java.naming ones we can add all of them to the array.

@jGauravGupta

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

jenkins test please

String systemPropertyName = key.replaceFirst("fish\\.payara\\.|java\\.naming\\.", "fish.payara.ejb.http.client.");
String value = System.getProperty(systemPropertyName, null);
for (String environmentVariable : SYSTEM_PROPERTY_KEYS) {
if (!environment.containsKey(environmentVariable)) {

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen May 15, 2019

Contributor

The method name update... suggests that you can actually update the environment map from system properties. But this if allows only to set missing values. Maybe complete... would be a better name for it?

This comment has been minimized.

Copy link
@jbee

jbee May 15, 2019

Author Contributor

renamed it.

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

jenkins test please

@pdudits
Copy link
Contributor

left a comment

I added "integration" test with JNDI to assure that it also works with usual way of getting a context (new InitialContext()), and I think everything's ok.

* The name of the property is the same as the environment variable name.
*/
private String[] SYSTEM_PROPERTY_KEYS = {
Context.INITIAL_CONTEXT_FACTORY,

This comment has been minimized.

Copy link
@pdudits

pdudits May 15, 2019

Contributor

System properties are read as Strings, but surely majority of factory's properties are of different type. But I realized they are converted in the context where reasonable

This comment has been minimized.

Copy link
@jbee

jbee May 15, 2019

Author Contributor

I had the same thought but all of these in the list are defined as some String value according to the javadoc that is converted later on.

@pdudits

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

jenkins test please

@MarkWareham MarkWareham merged commit 1a96de5 into payara:master May 16, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.