[JBEAP-26296] Allow specifying --repositories … #569

ChristinaDsl · 2024-03-01T13:03:18Z

https://issues.redhat.com/browse/JBEAP-26296

spyrkob

@ChristinaDsl that doesn't work, the local path has to be converted to an URL to be usable:

./prospero install --fpl org.wildfly.core:wildfly-core-galleon-pack:20.0.0.Beta3 \
  --manifest examples/wildfly-core-manifest.yaml --repositories test-repo \
  --dir installation-dir
Installing feature pack: org.wildfly.core:wildfly-core-galleon-pack:20.0.0.Beta3
Using channels:
# manifest: file:/Users/spyrkob/workspaces/set/prospero/prospero/examples/wildfly-core-manifest.yaml
  repositories:
    id: temp-repo-0
    url: test-repo



ERROR: The following artifacts could not be resolved: org.wildfly.core:wildfly-core-galleon-pack:zip:20.0.0.Beta3 (absent): Could not transfer artifact org.wildfly.core:wildfly-core-galleon-pack:zip:20.0.0.Beta3 from/to temp-repo-0 (test-repo): Cannot access test-repo with type default using the available connector factories: BasicRepositoryConnectorFactory

Also could you add unit tests to RepositoryDefinitionTest and an integration test to org.wildfly.prospero.it.cli.InstallTest to verify this change?

spyrkob · 2024-03-12T14:28:16Z

prospero-cli/src/test/java/org/wildfly/prospero/cli/RepositoryDefinitionTest.java

+            from(List.of(fileURL));
+            from(List.of("file://"+filePath));
+        } catch (Exception e) {
+            fail("Provided URL's absolute or relative path is not correct");


@ChristinaDsl you don't need to catch the exception. If it fails the test fails. Also you should check that the returned value is correct. The method might run without exception, but the returned value might be wrong

spyrkob · 2024-03-18T16:48:00Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+            if (text.startsWith("file://..")){
+                return false;
+            } else if (text.startsWith("file")){
+                if (Pattern.compile("/[A-Z]:").matcher(text).find()){


@ChristinaDsl why do you need this check? Paths.get(url.toURI() should work on both Windows and Linux paths

It does not work on relative paths. In windows and file URLs Paths.get() throws error and in Linux relative path cannot be converted initially in URL.

OK thank you. Could you add this explanation in a comment in the code please?

spyrkob · 2024-03-18T16:52:48Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

-            new URL(text);
+            URL url = new URL(text);
+            if (text.startsWith("file://..")){
+                return false;


Can you add a debug message why this is incorrect

spyrkob · 2024-03-18T16:54:18Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+                } else {
+                    path = Paths.get(url.getPath()).toAbsolutePath();
+                }
+                return new File(String.valueOf(path)).exists();


return Files.exists(path) or return path.toFile().exists()

spyrkob · 2024-03-18T16:55:52Z

integration-tests/src/test/java/org/wildfly/prospero/it/cli/InstallTest.java

+        final URL temporaryRepo = mockTemporaryRepo(true);
+
+        ExecutionUtils.prosperoExecution(CliConstants.Commands.UPDATE, CliConstants.Commands.PERFORM,
+                        CliConstants.REPOSITORIES, temporaryRepo.toString(),


the temporaryRepo is an URL, for this test to really verify this functionality it would have to be a relative path

Thanks for your comment. This test is to verify that a local repo can be found and the update command is indeed working when an argument " --repositories" is passed along with the url of local repo. So, why should be a relative path here?

There already are tests in UpdateWithAdditionalRepositoryTest that verify this use case. The new use case added in this issue should be that a user can pass a local path (e.g. ../test-repository or /home/user/test-repository) as an argument for --repositories

So check both for absolute and relative paths in this method?

ideally yes, but relative path would be enough

spyrkob · 2024-03-18T17:00:03Z

prospero-cli/src/test/java/org/wildfly/prospero/cli/RepositoryDefinitionTest.java

+    }
+
+    @Test
+    public void testCorrectRelativeOrAbsolutePathForFileURL() throws Exception {


what about a relative path?

The relative path is test in the first lines of this method with this url "file:../prospero-common"

That's a URI with relative path. A relative path would be ../prospero-common on its own

The whole method contains URLs and a URI, not just its absolute and relative paths. Should I change the whole method to contain only paths?

I believe this verifies JBEAP-26295 as well so those are valid tests. Just adding some examples of paths will be enough.

spyrkob · 2024-04-09T10:42:55Z

integration-tests/src/test/java/org/wildfly/prospero/it/cli/InstallTest.java

+
+        final URL temporaryRepo = mockTemporaryRepo(true);
+
+        Path relativePath = Paths.get(System.getProperty("user.dir")).relativize(Path.of(Path.of(temporaryRepo.toURI()).toUri()));


The nested Path.of is not needed

spyrkob · 2024-04-09T10:47:06Z

integration-tests/src/test/java/org/wildfly/prospero/it/cli/InstallTest.java

+        Path relativePath = Paths.get(System.getProperty("user.dir")).relativize(Path.of(Path.of(temporaryRepo.toURI()).toUri()));
+
+        ExecutionUtils.prosperoExecution(CliConstants.Commands.UPDATE, CliConstants.Commands.PERFORM,
+                        CliConstants.REPOSITORIES, ".."+File.separator+"integration-tests"+File.separator+relativePath,


You can use Path.of(String...) to build the path instead of appending the strings.

Also, relativePath is already a relative path, so I don't think adding ../integration-tests" to it brings anything to the test. I'd keep it simple and just use the relativePath` here

Adding "../integration-tests" is important because if I just use relative path then I have a result "target/folder1/folder2" which is invalid for two reasons:
Firstly, is not a relative path
Secondly, the folder the test exists is "integration-tests". So, if I just put "../"+relative path would go out of integration-tests folder and search the target inside parent folder which is "prospero". I need too search inside "integration-tests" the target folder because there it is created.

target/folder1/folder2 is a relative path. An absolute path starts with root directory (e.g. /home/jboss/ on linux or C:\Users\jboss on Windows). A relative path doesn't have a root folder and is relative to the current folder (e.g. target/foo is relative because it depends on where the user is)

spyrkob · 2024-04-09T10:48:24Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/CliMessages.java

@@ -418,6 +418,10 @@ default ArgumentParsingException invalidRepositoryDefinition(String repoKey) {
        return new ArgumentParsingException(format(bundle.getString("prospero.general.validation.repo_format"), repoKey));
    }

+    default ArgumentParsingException invalidFilePath(String repoKey) {


s/repoKey/path

spyrkob · 2024-04-09T10:50:13Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java


 public class RepositoryDefinition {

-    public static List<Repository> from(List<String> repos) throws ArgumentParsingException {
+    public static List<Repository> from(List<String> repos) throws ArgumentParsingException, MalformedURLException, URISyntaxException {


Can you catch the new exceptions and wrap them in ArgumentParsingException with an explanation message?

spyrkob · 2024-04-09T10:53:08Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+
+    private static boolean isValidUrl(String text) throws ArgumentParsingException {
+        if (text.startsWith("file://..")) {
+            Logger.getLogger("'file://../path/to/repo' is invalid as the format of file URL should be " +


Did you mean Logger.warn ?

spyrkob · 2024-04-09T10:56:17Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+       whereas for Linux, Paths.get(url.getPath()).toAbsolutePath() is more suitable. */
+    private static Path getPathFromURL(String text) throws MalformedURLException, URISyntaxException, ArgumentParsingException {
+        URL url = new URL(text);
+        if (Pattern.compile("/[A-Z]:").matcher(text).find()) {


That's not going to correct, e.g. in Linux it's possible to create a folder with name A:

iweiss · 2024-04-19T13:22:02Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

    public static List<Repository> from(List<String> repos) throws ArgumentParsingException {
        ArrayList<Repository> repositories = new ArrayList<>(repos.size());
        for (int i = 0; i < repos.size(); i++) {
-            final String text = repos.get(i);
+            String text = repos.get(i);


The variable can be more descriptive than text. Maybe repoName?

iweiss · 2024-04-19T13:22:37Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

@@ -56,4 +69,31 @@ private static boolean isValidUrl(String text) {
            return false;
        }
    }
+
+    public static URI getAbsoluteFileURI(String text) throws ArgumentParsingException, URISyntaxException {


Same as above.

iweiss · 2024-04-19T13:23:02Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+
+    public static URI getAbsoluteFileURI(String text) throws ArgumentParsingException, URISyntaxException {
+        String path;
+        path = getPath(text);


Why is the assignment on a separate line?

I will fix this

iweiss · 2024-04-19T13:23:53Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+    public static URI getAbsoluteFileURI(String text) throws ArgumentParsingException, URISyntaxException {
+        String path;
+        path = getPath(text);
+        File file = new File(path);


I think file can be a one-liner.
File file = new File(getPath(text));, no?

If file does not exist, the error message should include the path, so it is used in more than one line.

Do you mean, throw CliMessages.MESSAGES.invalidFilePath(path);? If so, you are already using path instead of file.

What do you mean?
file is also used in two lines,
if (file.exists()) {
return file.toURI();

if that is your point..

My point is that you don't need to create a new variable for the single purpose, path, to be used as a parameter for getPath() and then used to create a File object. You can simplify it with File file = new File(getPath(text)); for example.

iweiss · 2024-04-19T13:24:14Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+        }
+    }
+
+    public static String getPath(String text) throws URISyntaxException {


text, same as above.

iweiss · 2024-04-19T13:26:06Z

prospero-cli/src/test/java/org/wildfly/prospero/cli/commands/InstallCommandTest.java

@@ -241,37 +241,45 @@ public void provisionConfigAndChannelSet() throws IOException {
    @Test
    public void provisionConfigAndRemoteRepoSet() throws Exception {
        Path channelsFile = temporaryFolder.newFile().toPath();
+
+        File test = temporaryFolder.newFolder();
+        String testURL = test.toURI().toURL().toString();


Why converting to URI, then to URL and then to String?

To create a URL out of a file object. I guess we could skip last call .toURL()

iweiss · 2024-04-19T13:26:15Z

prospero-cli/src/test/java/org/wildfly/prospero/cli/commands/InstallCommandTest.java

+
+        File test = temporaryFolder.newFolder();
+        String testURL = test.toURI().toURL().toString();
+


Same as above

iweiss · 2024-04-19T13:26:53Z

prospero-cli/src/test/java/org/wildfly/prospero/cli/commands/InstallCommandTest.java

@@ -241,37 +241,45 @@ public void provisionConfigAndChannelSet() throws IOException {
    @Test
    public void provisionConfigAndRemoteRepoSet() throws Exception {
        Path channelsFile = temporaryFolder.newFile().toPath();
+
+        File test = temporaryFolder.newFolder();


Variable could have a more descriptive name

iweiss · 2024-04-19T13:26:57Z

prospero-cli/src/test/java/org/wildfly/prospero/cli/commands/InstallCommandTest.java

    }

    @SuppressWarnings("unchecked")
    @Test
    public void passShadowRepositories() throws Exception {
        Path channelsFile = temporaryFolder.newFile().toPath();
+
+        File test = temporaryFolder.newFolder();


Variable could have a more descriptive name

iweiss · 2024-04-22T09:22:19Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+    }
+
+    public static String getPath(String text) throws URISyntaxException {
+        if (text.startsWith("file")) {


I think this should be file: to make sure the user is inputting a URI and not a file named file_dir for example

spyrkob · 2024-04-29T13:05:35Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

-            final String text = repos.get(i);
-            if(text.contains("::")) {
-                final String[] parts = text.split("::");
+            String repoName = repos.get(i);


nitpick - the repoName variable name is a bit misleading - the string can either contain repoName::repoLocation or just repoLocation

spyrkob · 2024-04-29T13:08:49Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

                repositories.add(new Repository(parts[0], parts[1]));
            } else {
-                if (!isValidUrl(text)) {
-                    throw CliMessages.MESSAGES.invalidRepositoryDefinition(text);
+                if (!repoName.startsWith("http") && !repoName.isEmpty()) {


should check if it starts with either http:// or https://.

if it starts with "https", it should be handled differently?

Both http and https should be treated the same - as web URLs. The difference is that http://foo.bar is an URL http/foo/bar is a path

spyrkob · 2024-04-29T13:13:50Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/RepositoryDefinition.java

+            URI inputUri = new URI(repoName);
+            if (inputUri.getScheme().equals("file") &&
+                    !inputUri.getSchemeSpecificPart().startsWith("/")) {
+                inputUri = new URI(("file:/" +


IIUC at this point this is a relative URI. In which case Path.of(inputUri.getSchemeSpecificPart()).toAbsolutePath() should be all you need to get the absolute path

Also this already creates the Path from the URI, why does it need to be converted back to URI and then again to Path?

spyrkob · 2024-04-29T13:17:39Z

prospero-cli/src/main/java/org/wildfly/prospero/cli/commands/AbstractInstallCommand.java

@@ -100,7 +102,7 @@ protected MavenOptions getMavenOptions() throws ArgumentParsingException {
        return mavenOptions.build();
    }

-    protected ProvisioningDefinition.Builder buildDefinition() throws MetadataException, NoChannelException, ArgumentParsingException {
+    protected ProvisioningDefinition.Builder buildDefinition() throws MetadataException, NoChannelException, ArgumentParsingException, MalformedURLException, URISyntaxException {


Those exceptions should be wrapped in ArgumentParsingException

spyrkob · 2024-05-03T09:58:05Z

@ChristinaDsl please remove the [installation-manager] from commit description, and I think it'll be good to go.

…ocal paths

…alidated correctly

spyrkob requested changes Mar 1, 2024

View reviewed changes

ChristinaDsl force-pushed the JBEAP-26296 branch from e8d4b80 to c3fb1bb Compare March 11, 2024 14:38

ChristinaDsl requested a review from spyrkob March 12, 2024 08:18

ChristinaDsl force-pushed the JBEAP-26296 branch from c3fb1bb to 3a9e027 Compare March 12, 2024 11:38

spyrkob reviewed Mar 12, 2024

View reviewed changes

ChristinaDsl force-pushed the JBEAP-26296 branch 2 times, most recently from b4b7fb8 to 5e0742c Compare March 15, 2024 10:34

spyrkob requested changes Mar 18, 2024

View reviewed changes

ChristinaDsl force-pushed the JBEAP-26296 branch from 5e0742c to b47e196 Compare April 5, 2024 12:07

ChristinaDsl requested a review from spyrkob April 9, 2024 09:38

spyrkob reviewed Apr 9, 2024

View reviewed changes

ChristinaDsl force-pushed the JBEAP-26296 branch from b47e196 to 5cc91b3 Compare April 19, 2024 10:35

iweiss reviewed Apr 19, 2024

View reviewed changes

iweiss reviewed Apr 22, 2024

View reviewed changes

ChristinaDsl force-pushed the JBEAP-26296 branch from 5cc91b3 to b636752 Compare April 22, 2024 09:57

spyrkob requested changes Apr 29, 2024

View reviewed changes

ChristinaDsl force-pushed the JBEAP-26296 branch from b636752 to 4d1db4c Compare April 30, 2024 13:43

spyrkob force-pushed the JBEAP-26296 branch 2 times, most recently from eb8a7d4 to 4d1db4c Compare May 2, 2024 11:44

ChristinaDsl changed the title ~~[JBEAP-26296] [installation-manager] Allow specifying --repositories …~~ [JBEAP-26296] Allow specifying --repositories … May 8, 2024

ChristinaDsl added 2 commits May 13, 2024 12:47

[JBEAP-26296] Allow specifying --repositories without IDs and using l…

5043629

…ocal paths

[JBEAP-26295] The file URL passed as --repositories argument is not v…

9e415d8

…alidated correctly

ChristinaDsl force-pushed the JBEAP-26296 branch from 1f20448 to 9e415d8 Compare May 13, 2024 09:48

[JBEAP-26295] Fix the test and refactor code

71b0328

spyrkob approved these changes May 13, 2024

View reviewed changes

spyrkob merged commit 2952150 into wildfly-extras:main May 13, 2024
7 checks passed

spyrkob mentioned this pull request May 13, 2024

[JBEAP-26296] Allow specifying --repositories without IDs and using local paths #667

Merged


		final URL temporaryRepo = mockTemporaryRepo(true);

		Path relativePath = Paths.get(System.getProperty("user.dir")).relativize(Path.of(Path.of(temporaryRepo.toURI()).toUri()));


		File test = temporaryFolder.newFolder();
		String testURL = test.toURI().toURL().toString();

[JBEAP-26296] Allow specifying --repositories … #569

[JBEAP-26296] Allow specifying --repositories … #569

Conversation

ChristinaDsl commented Mar 1, 2024

spyrkob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristinaDsl Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristinaDsl Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristinaDsl Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyrkob Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyrkob commented May 3, 2024

ChristinaDsl Mar 19, 2024 •

edited

Loading

ChristinaDsl Mar 19, 2024 •

edited

Loading

ChristinaDsl Apr 22, 2024 •

edited

Loading

spyrkob Apr 29, 2024 •

edited

Loading