-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add Maven endpoint #337
base: development
Are you sure you want to change the base?
Add Maven endpoint #337
Conversation
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 have left a bunch of comments. The biggest issue is not handling multiple repos. This was the first bullet point in what needed done in my last review. It was the most important thing, even more important than semver ranges.
src/cfml/system/endpoints/Maven.cfc
Outdated
function init(){ | ||
setNamePrefixes( "maven" ); | ||
var orderedStruct = structNew( "ordered" ); | ||
orderedStruct[ "mavenCentral" ] = "https://maven-central.storage.googleapis.com/maven2/"; |
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 didn't intend for any of these to be bundled with CommandBOx outside of maven central. They were just examples of repos a user could register. These should be "opt-in" and only if the user configures them. All we need to know about by default is Maven central.
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.
got it.
src/cfml/system/endpoints/Maven.cfc
Outdated
boolean verbose = false | ||
){ | ||
var job = wirebox.getInstance( "interactiveJob" ); | ||
var repos = getGlobalRepos(); // Global linked struct of repos |
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.
Global configured repos were to come from config settings
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.
done
src/cfml/system/endpoints/Maven.cfc
Outdated
){ | ||
var job = wirebox.getInstance( "interactiveJob" ); | ||
var repos = getGlobalRepos(); // Global linked struct of repos | ||
// var projectRepos = {}; // TODO: Local overrides from box.json |
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 needs implemented. Read the box.json
in the currentWorkingDirectory
to find any project-specific repos.
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.
done
src/cfml/system/endpoints/Maven.cfc
Outdated
var job = wirebox.getInstance( "interactiveJob" ); | ||
var repos = getGlobalRepos(); // Global linked struct of repos | ||
// var projectRepos = {}; // TODO: Local overrides from box.json | ||
// var repos = structAppend(globalRepos, projectRepos, false); // Preserve order |
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.
overwrite needs to be true. A project repo of the same alias should overwrite a global repo of the same name.
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.
done
src/cfml/system/endpoints/Maven.cfc
Outdated
|
||
var artifactParts = getArtifactParts( package ); | ||
var jarFileURL = ""; | ||
var artifact = { |
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.
It feels like these 3 variables should be one. The jarfileURL is already contained in the artifact struct, and the artifact parts feel like they deserve to be stored in the artifact struct as well. Then the logic below can start filling in the missing pieces of information as-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.
done
src/cfml/system/endpoints/Maven.cfc
Outdated
// override the box.json with the actual version and dependencies | ||
var boxJSON = { | ||
"name" : "#artifactParts.groupId & "-" & artifactParts.artifactId#.jar", | ||
"slug" : artifactParts.artifactId, |
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.
Make this group and artifact ID
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.
done
src/cfml/system/endpoints/Maven.cfc
Outdated
var boxJSON = { | ||
"name" : "#artifactParts.groupId & "-" & artifactParts.artifactId#.jar", | ||
"slug" : artifactParts.artifactId, | ||
"version" : artifactParts.version, |
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 should be the actual version that was installed, not the incoming semver range
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.
that is actually the installed version. My logic updates the artifacts version above when resolving the range and I use it here.
src/cfml/system/endpoints/Maven.cfc
Outdated
continue; | ||
} | ||
dependencies[ dependency.artifactId ] = getNamePrefixes() & ( | ||
artifactParts.repo.len() ? artifactParts.repo & "|" : "" |
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'm not sure if the HTTP calls are processing this part of the main package's POM, but a pom.xml can register its own repo aliases to reference. If one of the dependencies points to an alias configured in the pom.xml's <repositories>
section, then we need to swap it out for the correct repo URL since those aliases won't be the same as what CommandBox has registered.
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 part needs some more work still. I will need to do more reading on the POM file to check for the repositories and aliases and swap the repo URL.
src/cfml/system/endpoints/Maven.cfc
Outdated
} | ||
dependencies[ dependency.artifactId ] = getNamePrefixes() & ( | ||
artifactParts.repo.len() ? artifactParts.repo & "|" : "" | ||
) & dependency.groupId & ":" & dependency.artifactId & ":" & dependency.version; |
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 may have missed it, but I can't find the logic where you are converting the Maven style of semantic version ranges to the CommandBox (npm) flavor.
src/cfml/system/endpoints/Maven.cfc
Outdated
scopes, | ||
depth++ | ||
); | ||
for ( v in d ) { |
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.
Please use full words for variable names.
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.
done
@quinteroj please address the issues. And ask questions if you don't understand please. It seems s lot what missed here. |
No description provided.