Conversation
bdw429s
left a comment
bdw429s
left a comment
There was a problem hiding this comment.
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.
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.
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.
Global configured repos were to come from config settings
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.
This needs implemented. Read the box.json in the currentWorkingDirectory to find any project-specific repos.
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.
overwrite needs to be true. A project repo of the same alias should overwrite a global repo of the same name.
src/cfml/system/endpoints/Maven.cfc
Outdated
|
|
||
| var artifactParts = getArtifactParts( package ); | ||
| var jarFileURL = ""; | ||
| var artifact = { |
There was a problem hiding this comment.
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.
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.
Make this group and artifact ID
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.
This should be the actual version that was installed, not the incoming semver range
There was a problem hiding this comment.
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.
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.
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.
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.
Please use full words for variable names.
|
@quinteroj please address the issues. And ask questions if you don't understand please. It seems s lot what missed here. |
3 participants
No description provided.