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

CANCELLED ARTEMIS-2412: Allow CF configuration through JNDI references #2746

Closed
wants to merge 2 commits into from
Closed

CANCELLED ARTEMIS-2412: Allow CF configuration through JNDI references #2746

wants to merge 2 commits into from

Conversation

boekhold
Copy link
Contributor

@boekhold boekhold commented Jul 9, 2019

Most connection related properties, like the SSL ones, currently
have to be encoded in the brokerURL. When configuring connections
purely through JNDI bindings, this is not always desireable.
This commit allows one to configure all properties included
in TransportConstants.ALLOWABLE_CONNECTOR_KEYS to be listed separately
in the JNDI bindings. These properties are then zipped into any
provided brokerURL. For properties that appear in both places,
the one specified separately in the JNDI bindings takes priority.

This commit should not affect any configuration other than those
configure through JNDIReferenceFactory.

@boekhold
Copy link
Contributor Author

boekhold commented Jul 9, 2019

FYI I think there's something wrong with the Travis-CI. I ran a 'mvn clean install' on my dev PC (well, IntelliJ equivalent), without encountering any errors. Also, the Travis-CI build seems to fail in a place I have not touched.

@jbertram
Copy link
Contributor

jbertram commented Jul 9, 2019

I ran the PR build on my local machine and it passed. This PR looks good, but I hesitate to merge it without any kind of test. Perhaps add a test to org.apache.activemq.artemis.tests.unit.jms.jndi.ObjectFactoryTest? This looks like the right area for testing this functionality. Nice work so far!

@boekhold
Copy link
Contributor Author

Test case added. Let me know if this is good enough...

@clebertsuconic
Copy link
Contributor

I'm sorry, but I can't merge this with a merge commit on your branch.

Your add test is a merge commit.

Please, keep your branch flat. it's impossible to work with merge commits on any PR on any project I know out there.

do the following at least:

git pull --rebase upstream master
git push origin -f <your-branch-name>

Other things missing:

a JIRA: and a JIRA comment on your commit

you should have a single commit on your PR.

Let me know if you need any help with git-fu on cleaning up your PR. I'm usually good on cleaning up myself with intermediate branches, but your case here was a bit difficult for me. It should be easier on your end since you know your changes.

thanks!

properties into brokerURL if they have been set on
ActiveMQConnectionFactory
Most connection related properties, like the SSL ones, currently
have to be encoded in the brokerURL. When configuring connections
purely through JNDI bindings, this is not always desireable.
This commit allows one to configure all properties included
in TransportConstants.ALLOWABLE_CONNECTOR_KEYS to be listed separately
in the JNDI bindings. These properties are then zipped into any
provided brokerURL. For properties that appear in both places,
the one specified separately in the JNDI bindings takes priority.

This commit should not affect any configuration other than those
configure through JNDIReferenceFactory.
@boekhold boekhold changed the title ARTEMIS-2412: Allow CF configuration through JNDI references CANCELLED ARTEMIS-2412: Allow CF configuration through JNDI references Jul 17, 2019
@boekhold
Copy link
Contributor Author

Closing this one, will submit a new PR with a single commit referencing JIRA

@boekhold boekhold closed this Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants