Skip to content

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Oct 29, 2019

Fixes: #4983

@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

@cescoffier @kenfinnigan given that the only proper way to test this is to add a test that includes %test.... and %prod.... in application.properties and invoke the project via mvn and java and the fact that those tests are slow hard to maintain, would you like me to add it or not?

@geoand geoand added this to the 0.27.0 milestone Oct 29, 2019
@gsmet
Copy link
Member

gsmet commented Oct 29, 2019

Stupid question: we don't have other areas doing the same mistake? It looks like an honest one.

@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

That's a great question, let me check

@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

We have a lot of them unfortunately... I am not sure what we should do so close to a release...

@kenfinnigan
Copy link
Member

Instead of tests, is there a maven plugin we can use that looks for the bad way to retrieve config and fails the build if it's present?

@kenfinnigan
Copy link
Member

I can fix this one as part of my REST TCK work, as it needs to be rebased anyway

@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

@kenfinnigan not sure, but that would fail master unfortunately because there are places where the other way of doing things is valid and is being used and some where I'm not sure. This close to release I am very hesitant to make such across the board changes

@kenfinnigan
Copy link
Member

Wasn't suggesting now, just trying to think of a way to prevent them from being added in the future

@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

@kenfinnigan yes, we will need to address that for sure

@geoand geoand force-pushed the rest-client-deployment-profile branch from 20e5c4a to 3b73864 Compare October 29, 2019 13:22
@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

CI failure seems to be due to Azure infra. I'll restart the failing tests once the others finish

@kenfinnigan
Copy link
Member

I've included this fix in the REST TCK fixes, so we could close

@geoand
Copy link
Contributor Author

geoand commented Oct 29, 2019

OK, closing as #4954 includes the fix this PR proposed

@geoand geoand closed this Oct 29, 2019
@geoand geoand deleted the rest-client-deployment-profile branch October 29, 2019 15:00
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.

rest-client doesn't take deployment profile when reading config

3 participants