-
Notifications
You must be signed in to change notification settings - Fork 328
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
Re-added pxf profile default to rpm and tar tasks #1301
Conversation
@@ -310,6 +315,7 @@ project('pxf-service') { | |||
|
|||
project.distTar { | |||
from('src/main/resources/pxf-profiles.xml') { into 'conf' } | |||
from('src/main/resources/pxf-profiles-default.xml') { into 'conf' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not work because pxf-profiles-default.xml has to be copied to pxf-profiles.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the product's perspective why is that needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, people would have to manually copy it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The purpose of pxf-profile-default.xml is to have all the default profiles. The users need to only add custom profiels to pxf-profile.xml. The PXF application loads both the xml files during startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why did it fail for pxf-automation on HAWQ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automation test failure is a different issue and that needs to be addressed separately out of this PR. This would not be an end user issue.
The automation test assumes pxf-profile.xml has all the profiles in it which is not really the case anymore. The tests need to addressed separately and not as part of the gradle file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not understand why pxf-profile.xml needs to be empty but that is a product question for @kongyew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sansanichfb just verified in the code that we are loading both profiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If pxf-private-default is not needed, then we should remove it. The less number of conf files we have, the better. |
After some investigation, the webapp does have pxf-profiles-default.xml file in pxf-service/webapps/pxf/WEB-INF/classes/ along with other configuration files. @lavjain is right, we dont' need pxf-profiles-default.xml in conf/. |
The tar and the rpm targets were missing pxf-profiles-default.xml.