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

ldap mock: using random port instead of hardcoded one #11140

Merged
merged 1 commit into from
Aug 20, 2020
Merged

ldap mock: using random port instead of hardcoded one #11140

merged 1 commit into from
Aug 20, 2020

Conversation

hartimcwildfly
Copy link

No description provided.

@geoand
Copy link
Contributor

geoand commented Aug 1, 2020

Thanks a lot for the contribution!

Would you like to explain what use case you are trying to solve with this?

@hartimcwildfly
Copy link
Author

I just think it's cleaner code to use a random port instead of a hardcoded one. And one does not have to manually set the quarkus.security.ldap.dir-context.url when using the LdapTestResource.

@geoand
Copy link
Contributor

geoand commented Aug 2, 2020

We don't really do this for other things, so I am kind of against this.

@gastaldi WDYT?

@gastaldi
Copy link
Contributor

gastaldi commented Aug 2, 2020

@geoand I don't have a strong opinion about this but given that this is used in tests and that the necessary parameters to run the tests are set in the LdapTestResource, I think that kinda makes sense.

Perhaps we should adapt our other tests to run in random ports in order to facilitate parallel testing

@maxandersen
Copy link
Member

we already do 8081 instead of 8080 for testing to run while products; I agree with @gastaldi it would be interesting to see what we can do to ensure parallel tests and less fricition in test setup is feasible.

@maxandersen
Copy link
Member

I don't know if random is the right approach - but I assume there gotta be many more examples of things like this ?

@gastaldi
Copy link
Contributor

gastaldi commented Aug 4, 2020

Random enables parallel tests compared to a fixed port, so I think it makes sense

@gastaldi
Copy link
Contributor

gastaldi commented Aug 4, 2020

we already do 8081 instead of 8080 for testing to run while products

We should look at enabling random ports in our HTTP tests, I think there is an issue asking for that already

@hartimcwildfly
Copy link
Author

Go for it!

@gsmet gsmet merged commit d13a380 into quarkusio:master Aug 20, 2020
@gsmet gsmet added this to the 1.8.0 - master milestone Aug 20, 2020
@gsmet
Copy link
Member

gsmet commented Aug 20, 2020

Merged, thanks!

@hartimcwildfly hartimcwildfly deleted the ldap-mock-using-random-instead-of-hardcoded-port branch August 20, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants