Skip to content

chor: Improve the code quality of the Registry class #55

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 52 additions & 32 deletions src/main/java/land/oras/Registry.java
Original file line number Diff line number Diff line change
Expand Up @@ -172,41 +172,20 @@ public void deleteBlob(ContainerRef containerRef) {
}
handleError(response);
}

/**
* Upload an ORAS artifact
* @param containerRef The container
* @param paths The paths
* @return The manifest
*/
public Manifest pushArtifact(ContainerRef containerRef, Path... paths) {
return pushArtifact(containerRef, null, Annotations.empty(), Config.empty(), paths);
}

/**
* Upload an ORAS artifact
* @param containerRef The container
* @param artifactType The artifact type
* @param paths The paths
* @return The manifest
*/
public Manifest pushArtifact(ContainerRef containerRef, String artifactType, Path... paths) {
return pushArtifact(containerRef, artifactType, Annotations.empty(), Config.empty(), paths);
}

/**
* Upload an ORAS artifact
* @param containerRef The container
* @param artifactType The artifact type
* @param annotations The annotations
* @param paths The paths
/**
* Push an ORAS artifact using the Builder pattern.
* @param builder The PushArtifactBuilder containing all configuration
* @return The manifest
*/
public Manifest pushArtifact(
ContainerRef containerRef, String artifactType, Annotations annotations, Path... paths) {
return pushArtifact(containerRef, artifactType, annotations, Config.empty(), paths);
public Manifest pushArtifact(PushArtifactBuilder builder) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be private ?

return pushArtifact(
builder.containerRef,
builder.artifactType,
builder.annotations,
builder.config,
builder.paths
);
}

/**
* Download an ORAS artifact
* @param containerRef The container
Expand Down Expand Up @@ -238,6 +217,7 @@ public void pullArtifact(ContainerRef containerRef, Path path, boolean overwrite
}
}


/**
* Upload an ORAS artifact
* @param containerRef The container
Expand Down Expand Up @@ -300,6 +280,46 @@ public Manifest pushArtifact(
LOG.debug("Manifest pushed to: {}", location);
return manifest;
}
/**
* Builder class for configuring artifact uploads.
*/
public static class PushArtifactBuilder {
private final ContainerRef containerRef; // Required parameter
private String artifactType = null; // Optional parameter
private Annotations annotations = Annotations.empty(); // Default value
private Config config = Config.empty(); // Default value
private Path[] paths; // Required parameter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not hood for user experience

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay thanks


// Constructor for required fields
public PushArtifactBuilder(ContainerRef containerRef, Path... paths) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be private?

this.containerRef = containerRef;
this.paths = paths;
}

// Setter for artifact type
public PushArtifactBuilder withArtifactType(String artifactType) {
this.artifactType = artifactType;
return this;
}

// Setter for annotations
public PushArtifactBuilder withAnnotations(Annotations annotations) {
this.annotations = annotations;
return this;
}

// Setter for config
public PushArtifactBuilder withConfig(Config config) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be private?

this.config = config;
return this;
}

// Build method (optional if you just want chaining)
public PushArtifactBuilder build() {
return this;
}
}


/**
* Push a blob from file
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/land/oras/RegistryTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ void testShouldPushAndPullMinimalArtifact() throws IOException {
Files.writeString(file1, "foobar");

// Upload
Manifest manifest = registry.pushArtifact(containerRef, file1);
Manifest manifest = registry.pushArtifact(new Registry.PushArtifactBuilder(containerRef, file1));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it's far from an improvement passing some object via new.

assertEquals(1, manifest.getLayers().size());

Layer layer = manifest.getLayers().get(0);
Expand Down Expand Up @@ -223,7 +223,7 @@ void testShouldPushCompressedDirectory() throws IOException {
Files.writeString(file3, "barfoo");

// Upload blob dir
Manifest manifest = registry.pushArtifact(containerRef, blobDir);
Manifest manifest = registry.pushArtifact(new Registry.PushArtifactBuilder(containerRef, blobDir));
assertEquals(1, manifest.getLayers().size());

Layer layer = manifest.getLayers().get(0);
Expand Down
Loading