Implement missing storage-cli operations for AliCloud #10

kathap · 2025-11-20T08:40:55Z

Add missing operations: copy, delete-recursive, list, properties, ensure-bucket-exists.

Missing Operations: copy, delete-recursive, list, properties, ensure-bucket-exists.

jochenehret · 2025-11-27T14:01:48Z

alioss/client/storage_client.go

 	) error

+	DeleteRecursive(
+		dest string,


What's meant with "dest" here? A bucket or a folder?

jochenehret · 2025-11-27T14:04:01Z

alioss/client/storage_client.go

-}

+	List(
+		prefix string,


Same as above: Is this a bucket name or can it also be a folder name?

jochenehret · 2025-11-27T14:13:09Z

alioss/client/storage_client.go

+	meta, err := dsc.bucket.GetObjectDetailedMeta(dest)
+	if err != nil {
+		var ossErr oss.ServiceError
+		if errors.As(err, &ossErr) && ossErr.StatusCode == 404 {


Does this mean the blob exists, but it has no properties?

jochenehret · 2025-11-27T14:22:20Z

alioss/client/storage_client.go

+			objects = append(objects, obj.Key)
+		}
+
+		if !resp.IsTruncated {


This is for pagination? Do we have a test for this?

Yes, this is for pagination, it keeps going until there are no more pages. IsTruncated = true if there are more objects to fetch. We will have a test for it ;)

jochenehret · 2025-11-27T14:24:46Z

alioss/main.go


+	case "copy":
+		if len(nonFlagArgs) != 3 {
+			log.Fatalf("Get method expected 3 arguments got %d\n", len(nonFlagArgs))


"Copy method expected ..." ?

jochenehret · 2025-11-27T14:28:24Z

alioss/main.go

+	case "delete-recursive":
+		var prefix string
+		if len(nonFlagArgs) > 2 {
+			log.Fatalf("delete-recursive takes at most one argument (prefix) got %d\n", len(nonFlagArgs)-1)


"one" or "two" arguments here?

Both is possible, "first" argument is the function name delete-recursive, if you don't specify a prefix it deletes all stuff in the current bucket (specified in config) and if you pass a "second" argument, that is the prefix and only objects starting with that prefix will be deleted in the current bucket. So it is "one" at most if you don't count the function name. Can add sth to the log line to make it clearer eg

delete-recursive takes at most one argument (prefix) got 2: [aa bb]

jochenehret · 2025-11-27T14:57:16Z

alioss/integration/general_ali_test.go

+		})
+	})
+
+	Describe("Invoking `ensure-bucket-exists`", func() {


Shouldn't there be a test that actually creates a bucket with this command and verifies that it exists?

jochenehret · 2025-11-27T15:00:26Z

alioss/integration/general_ali_test.go

+				Expect(err).ToNot(HaveOccurred())
+				Expect(cliSession.ExitCode()).To(BeZero())
+
+				cliSession, err = integration.RunCli(cliPath, configPath, "delete", destBlob)


If the srcBlob could not be deleted, the test panics and does not try to attempt to clean up the destBlob. Maybe just remove the Expects as this is not part of the expected test results?

Copilot

Pull request overview

This PR implements five missing storage-cli operations for AliCloud (Alibaba Cloud OSS): copy, delete-recursive, list, properties, and ensure-bucket-exists. These operations bring AliCloud feature parity with other storage providers in the storage-cli tool.

Key Changes

Added CLI command handlers for the five new operations with argument validation
Implemented the underlying storage client methods with pagination support for list and delete-recursive operations
Added comprehensive integration tests for all new operations
Modified the storage client initialization to cache OSS client and bucket instances

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
alioss/main.go	Added command handlers for copy, delete-recursive, list, properties, and ensure-bucket-exists with argument validation
alioss/client/storage_client.go	Implemented five new storage operations, modified struct to cache client/bucket instances, added BlobProperties type
alioss/client/client.go	Added wrapper methods in AliBlobstore to delegate to storage client implementations
alioss/integration/general_ali_test.go	Added integration tests for delete-recursive, copy, list, and ensure-bucket-exists operations
alioss/client/clientfakes/fake_storage_client.go	Auto-generated fake client code for new interface methods

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-27T15:48:51Z

alioss/main.go

+
+	case "ensure-bucket-exists":
+		if len(nonFlagArgs) != 1 {
+			log.Fatalf("EnsureBucketExists method expected 1 arguments got %d\n", len(nonFlagArgs))


Grammatical error: "expected 1 arguments" should be "expected 1 argument" (singular form).

Suggested change

log.Fatalf("EnsureBucketExists method expected 1 arguments got %d\n", len(nonFlagArgs))

log.Fatalf("EnsureBucketExists method expected 1 argument got %d\n", len(nonFlagArgs))

Copilot · 2025-11-27T15:48:52Z

alioss/main.go

+		} else if len(nonFlagArgs) == 2 {
+			prefix = nonFlagArgs[1]
+		} else {
+			log.Fatalf("List method expected 1 or 2 arguments, got %d\n", len(nonFlagArgs)-1)


The error message is misleading. It says "expected 1 or 2 arguments, got %d" and passes len(nonFlagArgs)-1, but this would exclude the command itself. For instance, if the user passes list prefix1 prefix2, len(nonFlagArgs) would be 3, and the message would say "got 2" which is incorrect - it should say "got 3" (command + 2 arguments). The format string should either say "expected 0 or 1 arguments" (excluding the command) or pass len(nonFlagArgs) instead of len(nonFlagArgs)-1.

Suggested change

log.Fatalf("List method expected 1 or 2 arguments, got %d\n", len(nonFlagArgs)-1)

log.Fatalf("List method expected 1 or 2 arguments, got %d\n", len(nonFlagArgs))

Copilot · 2025-11-27T15:48:52Z

alioss/client/client.go

+func (client *AliBlobstore) DeleteRecursive(prefix string) error {
+


[nitpick] Unnecessary blank line after function signature. This is inconsistent with other methods in the file (e.g., Delete at line 40, Get at line 36) which don't have blank lines before their return statements.

Copilot · 2025-11-27T15:48:53Z

alioss/client/client.go

+}
+
+func (client *AliBlobstore) Copy(srcBlob string, dstBlob string) error {
+


[nitpick] Unnecessary blank line after function signature. This is inconsistent with other methods in the file (e.g., Delete at line 40, Get at line 36) which don't have blank lines before their return statements.

Suggested change

Copilot · 2025-11-27T15:48:53Z

alioss/client/storage_client.go

 	storageConfig config.AliStorageConfig
+	client        *oss.Client
+	bucket        *oss.Bucket
+	bucketURL     string


The bucketURL field is set during initialization (line 90, 96) but is never used anywhere in the codebase. Consider removing it unless it's intended for future use.

Copilot · 2025-11-27T15:48:55Z

alioss/client/storage_client.go

 	) error

+	DeleteRecursive(
+		dest string,


Inconsistent parameter naming: The DeleteRecursive method uses dest as the parameter name (line 39), but this represents a prefix for filtering objects, not a destination. Consider renaming to prefix for clarity and consistency with the actual usage in the implementation (line 171) and the List method (line 57).

Suggested change

dest string,

prefix string,

Copilot · 2025-11-27T15:48:55Z

alioss/main.go

+	case "delete-recursive":
+		var prefix string
+		if len(nonFlagArgs) > 2 {
+			log.Fatalf("delete-recursive takes at most one argument (prefix) got %d\n", len(nonFlagArgs)-1)


The error message is misleading. It says "takes at most one argument (prefix) got %d" and passes len(nonFlagArgs)-1, but this logic is inconsistent. For instance, if the user passes delete-recursive prefix1 prefix2, len(nonFlagArgs) would be 3, and the message would say "got 2" arguments, which doesn't match the phrasing "takes at most one argument". The format string should either say "takes at most 2 total arguments" or pass len(nonFlagArgs)-1 with text like "got %d extra arguments".

Suggested change

log.Fatalf("delete-recursive takes at most one argument (prefix) got %d\n", len(nonFlagArgs)-1)

log.Fatalf("delete-recursive expected at most 2 arguments (command and optional prefix), got %d\n", len(nonFlagArgs))

Copilot · 2025-11-27T15:48:56Z

alioss/client/storage_client.go

+	) ([]string, error)
+
+	Properties(
+		dest string,


Inconsistent parameter naming: The Properties method uses dest as the parameter name, but this represents an object key to retrieve properties for, not a destination. Consider renaming to object for clarity and consistency with other methods like Delete (line 35) and Exists (line 43).

Suggested change

dest string,

object string,

Copilot · 2025-11-27T15:48:56Z

alioss/client/storage_client.go

+func (dsc DefaultStorageClient) Properties(
+	dest string,
+) error {
+	log.Printf("Getting properties for object %s/%s\n",
+		dsc.storageConfig.BucketName, dest)
+
+	meta, err := dsc.bucket.GetObjectDetailedMeta(dest)
+	if err != nil {
+		var ossErr oss.ServiceError
+		if errors.As(err, &ossErr) && ossErr.StatusCode == 404 {
+			fmt.Println(`{}`)
+			return nil
+		}
+
+		return fmt.Errorf("failed to get properties for object %s: %w", dest, err)
+	}
+
+	eTag := meta.Get("ETag")
+	lastModifiedStr := meta.Get("Last-Modified")
+	contentLengthStr := meta.Get("Content-Length")
+
+	var (
+		lastModified  time.Time
+		contentLength int64
+	)
+
+	if lastModifiedStr != "" {
+		t, parseErr := time.Parse(time.RFC1123, lastModifiedStr)
+		if parseErr == nil {
+			lastModified = t
+		}
+	}
+
+	if contentLengthStr != "" {
+		cl, convErr := strconv.ParseInt(contentLengthStr, 10, 64)
+		if convErr == nil {
+			contentLength = cl
+		}
+	}
+
+	props := BlobProperties{
+		ETag:          strings.Trim(eTag, `"`),
+		LastModified:  lastModified,
+		ContentLength: contentLength,
+	}
+
+	output, err := json.MarshalIndent(props, "", "  ")
+	if err != nil {
+		return fmt.Errorf("failed to marshal object properties: %w", err)
+	}
+
+	fmt.Println(string(output))
+	return nil
+}


Inconsistency: The Properties method uses dsc.bucket (the cached bucket from the struct), while existing methods like Upload (line 107-115) and Delete (line 157-165) create new client and bucket instances for each operation. This inconsistency could lead to unexpected behavior. Either use the cached bucket consistently across all methods, or create a new bucket instance in this method for consistency with existing code.

Copilot · 2025-11-27T15:48:56Z

alioss/client/client.go

+}
+
+func (client *AliBlobstore) Properties(dest string) error {
+


[nitpick] Unnecessary blank line after function signature. This is inconsistent with other methods in the file (e.g., Delete at line 40, Get at line 36) which don't have blank lines before their return statements.

Suggested change

kathap marked this pull request as draft November 20, 2025 08:41

cf-foundation-community-automation bot added this to Foundational Infrastructure Working Group Nov 20, 2025

cf-foundation-community-automation bot moved this to Inbox in Foundational Infrastructure Working Group Nov 20, 2025

kathap force-pushed the add-missing-functions-to-alioss branch from 565a6eb to 1699400 Compare November 25, 2025 12:56

kathap marked this pull request as ready for review November 25, 2025 17:12

stephanme mentioned this pull request Nov 26, 2025

[RFC0043] Cloud Controller Blobstore Type: storage-cli cloudfoundry/community#1274

Open

22 tasks

kathap added 3 commits November 26, 2025 10:29

Implement missing storage-cli operations for AliCloud

6e23540

Missing Operations: copy, delete-recursive, list, properties, ensure-bucket-exists.

add fake storage client

09538fe

clean up comments

2bfbdec

kathap force-pushed the add-missing-functions-to-alioss branch from f6f5c44 to 2bfbdec Compare November 26, 2025 09:29

jochenehret reviewed Nov 27, 2025

View reviewed changes

beyhan requested review from a team, peanball and serdarozerr and removed request for a team November 27, 2025 15:42

beyhan moved this from Inbox to Pending Review | Discussion in Foundational Infrastructure Working Group Nov 27, 2025

rkoster requested a review from Copilot November 27, 2025 15:42

Copilot started reviewing on behalf of rkoster November 27, 2025 15:43 View session

Copilot finished reviewing on behalf of rkoster November 27, 2025 15:46

Copilot AI reviewed Nov 27, 2025

View reviewed changes

	log.Fatalf("EnsureBucketExists method expected 1 arguments got %d\n", len(nonFlagArgs))
	log.Fatalf("EnsureBucketExists method expected 1 argument got %d\n", len(nonFlagArgs))

	log.Fatalf("List method expected 1 or 2 arguments, got %d\n", len(nonFlagArgs)-1)
	log.Fatalf("List method expected 1 or 2 arguments, got %d\n", len(nonFlagArgs))

		func (client *AliBlobstore) DeleteRecursive(prefix string) error {

		}

		func (client *AliBlobstore) Copy(srcBlob string, dstBlob string) error {

	log.Fatalf("delete-recursive takes at most one argument (prefix) got %d\n", len(nonFlagArgs)-1)
	log.Fatalf("delete-recursive expected at most 2 arguments (command and optional prefix), got %d\n", len(nonFlagArgs))

		}

		func (client *AliBlobstore) Properties(dest string) error {

Implement missing storage-cli operations for AliCloud #10

Are you sure you want to change the base?

Implement missing storage-cli operations for AliCloud #10

Conversation

kathap commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kathap Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kathap commented Nov 20, 2025 •

edited

Loading

kathap Dec 2, 2025 •

edited

Loading