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

Re-added pxf profile default to rpm and tar tasks #1301

Closed
wants to merge 1 commit into from

Conversation

shivzone
Copy link
Contributor

The tar and the rpm targets were missing pxf-profiles-default.xml.

@@ -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' }
Copy link

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

Copy link
Contributor Author

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 ?

Copy link

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?

Copy link
Contributor Author

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.

Copy link

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?

Copy link
Contributor Author

@shivzone shivzone Oct 12, 2017

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

Copy link

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

Copy link

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavjain pxf-profiles.xml is better to be empty to prevent users from incorrectly updating the default entires in it. This is similar to the separation we have between pxf-private.classpath from pxf-public.classpath.
@lavjain @sansanichfb does that mean we don't need pxf-profiles.default ?

@shivzone
Copy link
Contributor Author

@lavjain pxf-profiles.xml is better to be empty to prevent users from incorrectly updating the default entires in it. This is similar to the separation we have between pxf-private.classpath from pxf-public.classpath.
@lavjain @sansanichfb does that mean we don't need pxf-profiles.default ?

@lavjain
Copy link

lavjain commented Oct 13, 2017

If pxf-private-default is not needed, then we should remove it. The less number of conf files we have, the better.

@shivzone
Copy link
Contributor Author

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/.

@shivzone shivzone closed this Oct 13, 2017
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