Copilot commented on code in PR #4672:
URL: https://github.com/apache/solr/pull/4672#discussion_r3667458708
##########
solr/core/src/java/org/apache/solr/cli/CreateTool.java:
##########
@@ -314,6 +278,26 @@ protected void createCollection(CloudSolrClient
cloudSolrClient, CommandLine cli
echo(endMessage);
}
+ /** Zips the contents of a configset directory for upload via the Configsets
V2 API. */
+ private static byte[] zipConfigSet(Path confPath) throws IOException {
+ ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (ZipOutputStream zipOut = new ZipOutputStream(baos)) {
+ Files.walkFileTree(
+ confPath,
+ new SimpleFileVisitor<>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes
attrs)
+ throws IOException {
+ zipOut.putNextEntry(new
ZipEntry(confPath.relativize(file).toString()));
+ Files.copy(file, zipOut);
+ zipOut.closeEntry();
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ }
+ return baos.toByteArray();
+ }
Review Comment:
zipConfigSet currently (1) includes hidden/dotfiles (unlike the previous ZK
upload path, which excluded dotfiles via
ConfigSetService.UPLOAD_FILENAME_EXCLUDE_PATTERN) and (2) writes ZipEntry names
using Path.toString(), which can introduce platform-specific separators (e.g.,
backslashes on Windows). This can cause unexpected upload failures or
inconsistent configset contents across platforms. Consider mirroring
DownloadConfigSet.zipConfigSet’s behavior: skip hidden paths, include directory
entries, validate forbidden extensions, and normalize entry names to '/'
separators.
##########
solr/core/src/test/org/apache/solr/cli/CreateToolTest.java:
##########
@@ -52,4 +52,24 @@ public void testCreateCollectionWithBasicAuth() throws
Exception {
assertEquals(0, CLITestHelper.runTool(args, CreateTool.class));
}
+
+ @Test
+ public void testCreateCollectionUploadsNewConfigSet() throws Exception {
+ String[] args = {
+ "create",
+ "-c",
+ "testCreateCollectionUploadsNewConfigSet",
+ "-d",
+ configset("cloud-minimal").toString(),
+ "-n",
+ "cloud-minimal-uploaded",
+ "-z",
+ cluster.getZkClient().getZkServerAddress(),
+ "--credentials",
+ SecurityJson.USER_PASS,
+ "--verbose"
+ };
+
+ assertEquals(0, CLITestHelper.runTool(args, CreateTool.class));
Review Comment:
The new configset-upload test only asserts the CLI exit code, so it can pass
even if the upload path wasn’t exercised (or if the configset already existed).
Add a pre/post assertion that the configset znode was created under /configs to
make this test actually validate the new behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
