From 3e483fac53e3c93093c05ed4a6a380b7e3c5b050 Mon Sep 17 00:00:00 2001 From: Jenna Goldstrich Date: Mon, 17 Jun 2024 23:08:56 +0000 Subject: [PATCH 1/2] backport of commit 046c8f18692e8a051ca13770973dfe9e08590753 --- internal/hcp/api/mock_service.go | 23 +++++++---- internal/hcp/api/service_bucket.go | 22 ++++++---- internal/hcp/registry/types.bucket.go | 1 - .../hcp/registry/types.bucket_service_test.go | 41 ++++++++++++++----- internal/hcp/registry/types.bucket_test.go | 1 - 5 files changed, 57 insertions(+), 31 deletions(-) diff --git a/internal/hcp/api/mock_service.go b/internal/hcp/api/mock_service.go index 9a513705052..db2f64a7a1e 100644 --- a/internal/hcp/api/mock_service.go +++ b/internal/hcp/api/mock_service.go @@ -19,7 +19,7 @@ import ( // Upon calling a service method a boolean is set to true to indicate that a method has been called. // To skip the setting of these booleans set TrackCalledServiceMethods to false; defaults to true in NewMockPackerClientService(). type MockPackerClientService struct { - CreateBucketCalled, UpdateBucketCalled, BucketAlreadyExist bool + CreateBucketCalled, UpdateBucketCalled, GetBucketCalled, BucketNotFound bool CreateVersionCalled, GetVersionCalled, VersionAlreadyExist, VersionCompleted bool CreateBuildCalled, UpdateBuildCalled, ListBuildsCalled, BuildAlreadyDone bool TrackCalledServiceMethods bool @@ -55,14 +55,6 @@ func (svc *MockPackerClientService) PackerServiceCreateBucket( params *hcpPackerService.PackerServiceCreateBucketParams, _ runtime.ClientAuthInfoWriter, opts ...hcpPackerService.ClientOption, ) (*hcpPackerService.PackerServiceCreateBucketOK, error) { - - if svc.BucketAlreadyExist { - return nil, status.Error( - codes.AlreadyExists, - fmt.Sprintf("Code:%d %s", codes.AlreadyExists, codes.AlreadyExists.String()), - ) - } - if params.Body.Name == "" { return nil, errors.New("no bucket name was passed in") } @@ -84,6 +76,19 @@ func (svc *MockPackerClientService) PackerServiceCreateBucket( return ok, nil } +func (svc *MockPackerClientService) PackerServiceGetBucket( + params *hcpPackerService.PackerServiceGetBucketParams, _ runtime.ClientAuthInfoWriter, + opts ...hcpPackerService.ClientOption, +) (*hcpPackerService.PackerServiceGetBucketOK, error) { + if svc.TrackCalledServiceMethods { + svc.GetBucketCalled = true + } + if svc.BucketNotFound { + return nil, status.Error(codes.NotFound, fmt.Sprintf("Code:%d %s", codes.NotFound, codes.NotFound.String())) + } + return hcpPackerService.NewPackerServiceGetBucketOK(), nil +} + func (svc *MockPackerClientService) PackerServiceUpdateBucket( params *hcpPackerService.PackerServiceUpdateBucketParams, _ runtime.ClientAuthInfoWriter, opts ...hcpPackerService.ClientOption, diff --git a/internal/hcp/api/service_bucket.go b/internal/hcp/api/service_bucket.go index bfdbe99f809..c512f1deeea 100644 --- a/internal/hcp/api/service_bucket.go +++ b/internal/hcp/api/service_bucket.go @@ -36,20 +36,24 @@ func (c *Client) DeleteBucket( return c.Packer.PackerServiceDeleteBucket(deleteBktParams, nil) } -// UpsertBucket tries to create a bucket on a HCP Packer Registry. If the bucket exists it will -// handle the error and update the bucket with the provided details. +// UpsertBucket will create or update a bucket. It calls GetBucket first, if the bucket is not found it creates that bucket +// If GetBucket succeeded we then call UpdateBucket description and bucket labels in case they've changed. +// GetBucket is used instead of CreateBucket since users with bucket level access to specific existing buckets can not create new buckets. func (c *Client) UpsertBucket( ctx context.Context, bucketName, bucketDescription string, bucketLabels map[string]string, ) error { - // Create bucket if exist we continue as is, eventually we want to treat this like an upsert. - _, err := c.CreateBucket(ctx, bucketName, bucketDescription, bucketLabels) - if err != nil && !CheckErrorCode(err, codes.AlreadyExists) { - return err - } + getParams := hcpPackerService.NewPackerServiceGetBucketParamsWithContext(ctx) + getParams.LocationOrganizationID = c.OrganizationID + getParams.LocationProjectID = c.ProjectID + getParams.BucketName = bucketName - if err == nil { - return nil + _, err := c.Packer.PackerServiceGetBucket(getParams, nil) + if err != nil { + if CheckErrorCode(err, codes.NotFound) { + _, err = c.CreateBucket(ctx, bucketName, bucketDescription, bucketLabels) + } + return err } params := hcpPackerService.NewPackerServiceUpdateBucketParamsWithContext(ctx) diff --git a/internal/hcp/registry/types.bucket.go b/internal/hcp/registry/types.bucket.go index 9ebf42eb797..17f3e34c027 100644 --- a/internal/hcp/registry/types.bucket.go +++ b/internal/hcp/registry/types.bucket.go @@ -122,7 +122,6 @@ func (bucket *Bucket) Initialize( } bucket.Destination = fmt.Sprintf("%s/%s", bucket.client.OrganizationID, bucket.client.ProjectID) - err := bucket.client.UpsertBucket(ctx, bucket.Name, bucket.Description, bucket.BucketLabels) if err != nil { return fmt.Errorf("failed to initialize bucket %q: %w", bucket.Name, err) diff --git a/internal/hcp/registry/types.bucket_service_test.go b/internal/hcp/registry/types.bucket_service_test.go index a89233d686a..6195282038d 100644 --- a/internal/hcp/registry/types.bucket_service_test.go +++ b/internal/hcp/registry/types.bucket_service_test.go @@ -13,6 +13,7 @@ import ( func TestInitialize_NewBucketNewVersion(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() + mockService.BucketNotFound = true b := &Bucket{ Name: "TestBucket", @@ -34,10 +35,15 @@ func TestInitialize_NewBucketNewVersion(t *testing.T) { t.Errorf("unexpected failure: %v", err) } + if !mockService.GetBucketCalled { + t.Errorf("expected a call to GetBucket but it didn't happen") + } if !mockService.CreateBucketCalled { t.Errorf("expected a call to CreateBucket but it didn't happen") } - + if mockService.UpdateBucketCalled { + t.Errorf("unexpected call to UpdateBucket") + } if !mockService.CreateVersionCalled { t.Errorf("expected a call to CreateVersion but it didn't happen") } @@ -65,8 +71,6 @@ func TestInitialize_NewBucketNewVersion(t *testing.T) { func TestInitialize_UnsetTemplateTypeError(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true - b := &Bucket{ Name: "TestBucket", client: &hcpPackerAPI.Client{ @@ -90,7 +94,6 @@ func TestInitialize_UnsetTemplateTypeError(t *testing.T) { func TestInitialize_ExistingBucketNewVersion(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true b := &Bucket{ Name: "TestBucket", @@ -110,6 +113,12 @@ func TestInitialize_ExistingBucketNewVersion(t *testing.T) { if err != nil { t.Errorf("unexpected failure: %v", err) } + if !mockService.GetBucketCalled { + t.Errorf("expected call to GetBucket but it didn't happen") + } + if mockService.CreateBucketCalled { + t.Errorf("expected call to CreateBucket but it didn't happen") + } if !mockService.UpdateBucketCalled { t.Errorf("expected call to UpdateBucket but it didn't happen") @@ -143,7 +152,6 @@ func TestInitialize_ExistingBucketNewVersion(t *testing.T) { func TestInitialize_ExistingBucketExistingVersion(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true mockService.VersionAlreadyExist = true b := &Bucket{ @@ -175,12 +183,18 @@ func TestInitialize_ExistingBucketExistingVersion(t *testing.T) { t.Errorf("unexpected call to CreateBucket") } + if !mockService.GetBucketCalled { + t.Errorf("expected call to GetBucket but it didn't happen") + } + if mockService.CreateBucketCalled { + t.Errorf("unexpected call to CreateBucket") + } if !mockService.UpdateBucketCalled { t.Errorf("expected call to UpdateBucket but it didn't happen") } if mockService.CreateVersionCalled { - t.Errorf("unexpected a call to CreateVersion") + t.Errorf("unexpected call to CreateVersion") } if !mockService.GetVersionCalled { @@ -188,7 +202,7 @@ func TestInitialize_ExistingBucketExistingVersion(t *testing.T) { } if mockService.CreateBuildCalled { - t.Errorf("unexpected a call to CreateBuild") + t.Errorf("unexpected call to CreateBuild") } if b.Version.ID != "version-id" { @@ -212,7 +226,6 @@ func TestInitialize_ExistingBucketExistingVersion(t *testing.T) { func TestInitialize_ExistingBucketCompleteVersion(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true mockService.VersionAlreadyExist = true mockService.VersionCompleted = true mockService.BuildAlreadyDone = true @@ -238,6 +251,15 @@ func TestInitialize_ExistingBucketCompleteVersion(t *testing.T) { t.Errorf("unexpected failure: %v", err) } + if !mockService.GetBucketCalled { + t.Errorf("expected call to GetBucket, but it didn't happen") + } + if !mockService.UpdateBucketCalled { + t.Errorf("expected call to UpdateBucket, but it didn't happen") + } + if mockService.CreateBucketCalled { + t.Errorf("unexpected call to CreateBucket") + } if mockService.CreateVersionCalled { t.Errorf("unexpected call to CreateVersion") } @@ -257,7 +279,6 @@ func TestInitialize_ExistingBucketCompleteVersion(t *testing.T) { func TestUpdateBuildStatus(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true mockService.VersionAlreadyExist = true b := &Bucket{ @@ -310,9 +331,7 @@ func TestUpdateBuildStatus(t *testing.T) { func TestUpdateBuildStatus_DONENoImages(t *testing.T) { mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true mockService.VersionAlreadyExist = true - b := &Bucket{ Name: "TestBucket", client: &hcpPackerAPI.Client{ diff --git a/internal/hcp/registry/types.bucket_test.go b/internal/hcp/registry/types.bucket_test.go index 1b50fb887bf..194a0c27d8e 100644 --- a/internal/hcp/registry/types.bucket_test.go +++ b/internal/hcp/registry/types.bucket_test.go @@ -291,7 +291,6 @@ func TestBucket_PopulateVersion(t *testing.T) { t.Setenv("HCP_PACKER_BUILD_FINGERPRINT", "test-run-"+strconv.Itoa(i)) mockService := hcpPackerAPI.NewMockPackerClientService() - mockService.BucketAlreadyExist = true mockService.VersionAlreadyExist = true mockService.BuildAlreadyDone = tt.buildCompleted From 780d70eff79313d2b1634d7d71405576d449d8f0 Mon Sep 17 00:00:00 2001 From: Jenna Goldstrich Date: Tue, 18 Jun 2024 21:47:44 +0000 Subject: [PATCH 2/2] backport of commit 96e8421d1d0cbc17adbfdcc09f3ca28fa959342a --- internal/hcp/registry/types.bucket_service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/hcp/registry/types.bucket_service_test.go b/internal/hcp/registry/types.bucket_service_test.go index 6195282038d..16d046980e4 100644 --- a/internal/hcp/registry/types.bucket_service_test.go +++ b/internal/hcp/registry/types.bucket_service_test.go @@ -117,7 +117,7 @@ func TestInitialize_ExistingBucketNewVersion(t *testing.T) { t.Errorf("expected call to GetBucket but it didn't happen") } if mockService.CreateBucketCalled { - t.Errorf("expected call to CreateBucket but it didn't happen") + t.Errorf("unexpected call to CreateBucket") } if !mockService.UpdateBucketCalled {