enhancement as proposed from review

Author: kathap

alioss/client/client.go

@@ -42,7 +42,6 @@ func (client *AliBlobstore) Delete(object string) error {
 }
 
 func (client *AliBlobstore) DeleteRecursive(prefix string) error {
-
 	return client.storageClient.DeleteRecursive(prefix)
 }
 
@@ -86,16 +85,13 @@ func (client *AliBlobstore) List(prefix string) ([]string, error) {
 }
 
 func (client *AliBlobstore) Copy(srcBlob string, dstBlob string) error {
-
 	return client.storageClient.Copy(srcBlob, dstBlob)
 }
 
 func (client *AliBlobstore) Properties(dest string) error {
-
 	return client.storageClient.Properties(dest)
 }
 
 func (client *AliBlobstore) EnsureBucketExists() error {
-
 	return client.storageClient.EnsureBucketExists()
 }

alioss/client/storage_client.go

@@ -36,7 +36,7 @@ type StorageClient interface {
 	) error
 
 	DeleteRecursive(
-		dest string,
+		objects string,
 	) error
 
 	Exists(
@@ -54,11 +54,11 @@ type StorageClient interface {
 	) (string, error)
 
 	List(
-		prefix string,
+		bucketPrefix string,
 	) ([]string, error)
 
 	Properties(
-		dest string,
+		object string,
 	) error
 
 	EnsureBucketExists() error
@@ -137,13 +137,13 @@ func (dsc DefaultStorageClient) Download(
 }
 
 func (dsc DefaultStorageClient) Copy(
-	srcObject string,
-	destObject string,
+	sourceObject string,
+	destinationObject string,
 ) error {
-	log.Printf("Copying object from %s to %s", srcObject, destObject)
+	log.Printf("Copying object from %s to %s", sourceObject, destinationObject)
 
-	if _, err := dsc.bucket.CopyObject(srcObject, destObject); err != nil {
-		return fmt.Errorf("failed to copy object from %s to %s: %w", srcObject, destObject, err)
+	if _, err := dsc.bucket.CopyObject(sourceObject, destinationObject); err != nil {
+		return fmt.Errorf("failed to copy object from %s to %s: %w", sourceObject, destinationObject, err)
 	}
 
 	return nil
@@ -168,11 +168,11 @@ func (dsc DefaultStorageClient) Delete(
 }
 
 func (dsc DefaultStorageClient) DeleteRecursive(
-	prefix string,
+	objectPrefix string,
 ) error {
-	if prefix != "" {
+	if objectPrefix != "" {
 		log.Printf("Deleting all objects in bucket %s with prefix '%s'\n",
-			dsc.storageConfig.BucketName, prefix)
+			dsc.storageConfig.BucketName, objectPrefix)
 	} else {
 		log.Printf("Deleting all objects in bucket %s\n",
 			dsc.storageConfig.BucketName)
@@ -182,8 +182,8 @@ func (dsc DefaultStorageClient) DeleteRecursive(
 
 	for {
 		var listOptions []oss.Option
-		if prefix != "" {
-			listOptions = append(listOptions, oss.Prefix(prefix))
+		if objectPrefix != "" {
+			listOptions = append(listOptions, oss.Prefix(objectPrefix))
 		}
 		if marker != "" {
 			listOptions = append(listOptions, oss.Marker(marker))
@@ -326,20 +326,20 @@ type BlobProperties struct {
 }
 
 func (dsc DefaultStorageClient) Properties(
-	dest string,
+	bucketObject string,
 ) error {
 	log.Printf("Getting properties for object %s/%s\n",
-		dsc.storageConfig.BucketName, dest)
+		dsc.storageConfig.BucketName, bucketObject)
 
-	meta, err := dsc.bucket.GetObjectDetailedMeta(dest)
+	meta, err := dsc.bucket.GetObjectDetailedMeta(bucketObject)
 	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)
+		return fmt.Errorf("failed to get properties for object %s: %w", bucketObject, err)
 	}
 
 	eTag := meta.Get("ETag")

alioss/integration/general_ali_test.go

@@ -2,9 +2,11 @@ package integration_test
 
 import (
 	"bytes"
+	"fmt"
 
 	"os"
 
+	"github.com/aliyun/aliyun-oss-go-sdk/oss"
 	"github.com/cloudfoundry/storage-cli/alioss/config"
 	"github.com/cloudfoundry/storage-cli/alioss/integration"
 
@@ -209,13 +211,16 @@ var _ = Describe("General testing for all Ali regions", func() {
 			destBlob := blobName + "-dest"
 
 			defer func() {
-				cliSession, err := integration.RunCli(cliPath, configPath, "delete", srcBlob)
-				Expect(err).ToNot(HaveOccurred())
-				Expect(cliSession.ExitCode()).To(BeZero())
-
-				cliSession, err = integration.RunCli(cliPath, configPath, "delete", destBlob)
-				Expect(err).ToNot(HaveOccurred())
-				Expect(cliSession.ExitCode()).To(BeZero())
+				for _, b := range []string{srcBlob, destBlob} {
+					cliSession, err := integration.RunCli(cliPath, configPath, "delete", b)
+					if err != nil {
+						GinkgoWriter.Printf("cleanup: error deleting %s: %v\n", b, err)
+						continue
+					}
+					if cliSession.ExitCode() != 0 && cliSession.ExitCode() != 3 {
+						GinkgoWriter.Printf("cleanup: delete %s exited with code %d\n", b, cliSession.ExitCode())
+					}
+				}
 			}()
 
 			contentFile = integration.MakeContentFile("copied content")
@@ -346,9 +351,81 @@ var _ = Describe("General testing for all Ali regions", func() {
 			Expect(output).To(ContainSubstring(blob2))
 			Expect(output).NotTo(ContainSubstring(otherBlob))
 		})
+
+		It("lists all blobs across multiple pages", func() {
+			prefix := integration.GenerateRandomString()
+			const totalObjects = 120
+
+			var blobNames []string
+			var contentFiles []string
+
+			for i := 0; i < totalObjects; i++ {
+				blobName := fmt.Sprintf("%s/%03d", prefix, i)
+				blobNames = append(blobNames, blobName)
+
+				contentFile := integration.MakeContentFile(fmt.Sprintf("content-%d", i))
+				contentFiles = append(contentFiles, contentFile)
+
+				cliSession, err := integration.RunCli(cliPath, configPath, "put", contentFile, blobName)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(cliSession.ExitCode()).To(BeZero())
+			}
+
+			defer func() {
+				for _, f := range contentFiles {
+					_ = os.Remove(f) //nolint:errcheck
+				}
+
+				for _, b := range blobNames {
+					cliSession, err := integration.RunCli(cliPath, configPath, "delete", b)
+					if err == nil && (cliSession.ExitCode() == 0 || cliSession.ExitCode() == 3) {
+						continue
+					}
+				}
+			}()
+
+			cliSession, err := integration.RunCli(cliPath, configPath, "list", prefix)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(cliSession.ExitCode()).To(BeZero())
+
+			output := bytes.NewBuffer(cliSession.Out.Contents()).String()
+
+			for _, b := range blobNames {
+				Expect(output).To(ContainSubstring(b))
+			}
+		})
 	})
 
 	Describe("Invoking `ensure-bucket-exists`", func() {
+		It("creates a bucket that can be observed via the OSS API", func() {
+			newBucketName := bucketName + "-bommel"
+
+			cfg := defaultConfig
+			cfg.BucketName = newBucketName
+
+			configPath = integration.MakeConfigFile(&cfg)
+			defer func() { _ = os.Remove(configPath) }() //nolint:errcheck
+
+			ossClient, err := oss.New(cfg.Endpoint, cfg.AccessKeyID, cfg.AccessKeySecret)
+			Expect(err).ToNot(HaveOccurred())
+
+			defer func() {
+				if err := ossClient.DeleteBucket(newBucketName); err != nil {
+					if _, ferr := fmt.Fprintf(GinkgoWriter, "cleanup: failed to delete bucket %s: %v\n", newBucketName, err); ferr != nil {
+						GinkgoWriter.Printf("cleanup: failed to write cleanup message: %v\n", ferr)
+					}
+				}
+			}()
+
+			s1, err := integration.RunCli(cliPath, configPath, "ensure-bucket-exists")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(s1.ExitCode()).To(BeZero())
+
+			exists, err := ossClient.IsBucketExist(newBucketName)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeTrue())
+		})
+
 		It("is idempotent", func() {
 			s1, err := integration.RunCli(cliPath, configPath, "ensure-bucket-exists")
 			Expect(err).ToNot(HaveOccurred())