FISH-886 Deploy GAV from local repository + some code cleanup #5035
Conversation
|
jenkins test |
lprimak
added
PR: CLA
|
|
Just copyright year to 2020 |
|
@svendiedrichsen can you review this please? |
|
jenkins test |
AlanRoth
changed the title
AlanRoth
changed the title
| validURLFound = true; | ||
| } | ||
| } else { | ||
| HttpURLConnection httpConnection = (HttpURLConnection) artefactURL.openConnection(); |
There was a problem hiding this comment.
This makes some assumption about the type of connection but it might be a JarURLConnection. Like this one: jar:http://www.foo.com/bar/baz.jar.
You might want a separate if-block to handle these kind of archive URLs.
But if it is all about local URLs this is unnessessary.
There was a problem hiding this comment.
I only needed a local repository but jar scheme support also possible
There was a problem hiding this comment.
This needs to be decided by someone from Payara.
There was a problem hiding this comment.
I'd say that's outside the scope of this PR.
I'm not immediately sure of the need to handle them either tbh - I think that functionality is covered by the standard --deploy option unless you're talking about using a Jar Url as a "maven" repository in its own right, which sounds mega-niche!
| @@ -117,7 +120,7 @@ | |||
| * the provided GAV as Strings | |||
| */ | |||
| private Map<String, String> splitGAV(String GAV) throws MalformedURLException { | |||
| final String[] splitGAV = GAV.split(",|:"); | |||
| final String[] splitGAV = GAV.split("[,:]"); | |||
| final Map<String, String> GAVMap = new HashMap<>(); | |||
| try { | |||
| GAVMap.put("groupId", splitGAV[0].replace('.', '/')); | |||
There was a problem hiding this comment.
Maven coordinates might also include packaging and classifier. If the packaging coordinate exists you don't have to iterate over the whole list of possible file extensions. You could simply use this. If the classifier coordinate exists you won't be able to find the artefact without it in the URL.
You can find some examples here: https://developpaper.com/coordinates-of-maven-basic-course/
There was a problem hiding this comment.
This may be implemented if we stick to some predefined format. In community doc defined artifactId, groupId and version only. Link in next answer
There was a problem hiding this comment.
Yes, its true according to the Payara doc but shouldn't there be a possibility to use packaging and qualifier? You could imagine having a build of an deplyoable artefact which builts a jar and a war of the same name. According the current code it would only be possible to deploy the jar. As this comes first in the list. The same goes for the qualifier by which you have a built which produces two jars of the same name but one has i.e. the qualifier UBER to mark the uber-jar containing all the dependencies inside. You couldn't distinguish them without using a qualifier.
You could default those two GAV components to jar and empty and would not have to iterate over the list of file suffixes. But maybe I'm just over-engineering this thing for this usecase.
This needs to be decided by someone from Payara.
There was a problem hiding this comment.
Not allowing a user to specify packaging and qualifiers is simply a limitation / missing feature of the implementation at the moment - I'd say it's out of scope for this PR though. The use case you described makes sense, I just wouldn't bundle it into this PR to prevent scope creep, particularly since it isn't a trivial 1 line change.
| @@ -117,7 +120,7 @@ | |||
| * the provided GAV as Strings | |||
| */ | |||
| private Map<String, String> splitGAV(String GAV) throws MalformedURLException { | |||
| final String[] splitGAV = GAV.split(",|:"); | |||
| final String[] splitGAV = GAV.split("[,:]"); | |||
There was a problem hiding this comment.
Isn't a valid GAV only split by :? Doesn't it suggest when you find a comma that this is really a list of GAV? And if you accept lists here you should really focus on splitting by comma first to have a chance to determine optional fields like packaging and classifier.
There was a problem hiding this comment.
According to documentation https://docs.payara.fish/community/docs/5.2020.7/documentation/payara-micro/deploying/deploy-cmd-line.html#deploying-applications-from-a-maven-repository for each GAV needs its own --deployFromGAV option. Or I misunderstood the documentation. Here I just replaced logical operator with a simple class
There was a problem hiding this comment.
Yes it is true for the Payara doc, but at least uncommon to use a comma nonetheless. This needs to be decided by someone from Payara.
There was a problem hiding this comment.
Looking back at the commit history around this, comma was the original delimiter as that's the "default" delimiter for Payara CLI (though we do use different delimiters all over the place), and we added in support for using a colon after to be more in line with the usual Maven notation.
So fish.payara,guppy,1.0 and fish.payara:guppy:1.0 are both valid, and if you want to deploy more than one artefact you have to specify the option multiple times.
@lprimak I have done it. Someone from Payara needs to look into this to make some decisions though. |
|
@svendiedrichsen I will take a look in the next few days, thanks! |
…#292) * Deploy GAV from local repository * Change copyright year Co-authored-by: Alexander Pinchuk <[email protected]>
Description
Added ability to deploy GAV from local repository.
Example:
java -jar payara-micro.jar --deployFromGAV org.example:someapp:1.0 --additionalRepository file:///c:/Users/alex/.m2/repositoryImportant Info
Blockers
Testing
New tests
Testing Performed
Testing Environment
Zulu JDK 8, Windows 10 Pro
Documentation
Notes for Reviewers