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

[BZ-1323310] allow custom user maven settings in kie-ci to also accept an URL #719

Merged
merged 1 commit into from
Apr 7, 2016

Conversation

mariofusco
Copy link
Contributor

@mariofusco mariofusco commented Apr 6, 2016

No description provided.

@mariofusco
Copy link
Contributor Author

@etirelli according to what written by Matthew in his last comment https://bugzilla.redhat.com/show_bug.cgi?id=1323310#c4 one of the necessary step to allow seamless integration between kie-ci and Fabric is allowing to also pass a URL (other than a regular java File) as custom user settings.

I'm not sure if this change will be enough to close that issue (still waiting for Matthew's reply about that), but in all case I believe that this is a nice and useful improvement for kie-ci, also regardless of Fabric integration.

@mariofusco
Copy link
Contributor Author

ok to test

@etirelli
Copy link
Contributor

etirelli commented Apr 6, 2016

Looks good, +1 to merge.

@mariofusco mariofusco merged commit 449b5d2 into apache:master Apr 7, 2016
@mariofusco mariofusco deleted the bz1323310 branch April 7, 2016 07:05
@@ -111,8 +113,8 @@ protected MavenExecutionRequest buildMavenExecutionRequest( MavenRequest mavenRe
mavenExecutionRequest.setGlobalSettingsFile( new File( mavenRequest.getGlobalSettingsFile() ) );
}

if ( mavenRequest.getUserSettingsFile() != null ) {
mavenExecutionRequest.setUserSettingsFile( new File( mavenRequest.getUserSettingsFile() ) );
if ( mavenRequest.getUserSettingsSource() instanceof FileSettingsSource ) {

Choose a reason for hiding this comment

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

@mariofusco hi Mario, shouldn't this code be changed to avoid the instanceof call and instead always build a new temporary/cached file from the generic InputStream that class SettingSource exposes?

@mariofusco
Copy link
Contributor Author

@paoloantinori you're right, I will implement this fix. Anyway this is totally unrelated with the issue reported in the bugzilla, correct?

@paoloantinori
Copy link

Well, tbh I'm not entirely sure. Since it's using only FileSettingsSource what happens if the settings were specified with the newer support for return new UrlSettingsSource( new URL( customSettings ) ); ?

Is it losing information in that case?

@mariofusco
Copy link
Contributor Author

@paoloantinori That MavenEmbedder is used only when parsing a kjar's pom to find the dependencies eventually declared in parent poms, so it shouldn't have any impact on retrieval of a single kjar as reported in that ticket. Anyway I agree this has to be fixed, I'll push a fix asap.

@paoloantinori
Copy link

I see. Ok!

On Fri, Apr 8, 2016 at 2:42 PM, Mario Fusco notifications@github.com
wrote:

@paoloantinori https://github.com/paoloantinori That MavenEmbedder is
used only when parsing a kjar's pom to find the dependencies eventually
declared in parent poms, so it shouldn't have any impact on retrieval of a
single kjar as reported in that ticket. Anyway I agree this has to be
fixed, I'll push a fix asap.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#719 (comment)

Rikkola pushed a commit to Rikkola/drools that referenced this pull request Jan 27, 2020
dupliaka pushed a commit to dupliaka/drools that referenced this pull request Apr 1, 2022
cimbalek pushed a commit to cimbalek/incubator-kie-drools that referenced this pull request Jan 19, 2024
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.

None yet

3 participants