add functions CheckQueueExists CheckBucketExists #183

bpeng · 2024-11-27T00:09:35Z

Proposed Changes

Ticket https://github.com/GeoNet/tickets/issues/13878

Changes proposed in this pull request:

add functions to check bucket/queue exist

Production Changes

The following production changes are required to deploy these changes:

None

Review

Check the box that applies to this code review. If necessary please seek help with adding a checklist guide for the reviewer.
When assigning the code review please consider the expertise needed to review the changes.

This is a content (documentation, web page etc) only change.
This is a minor change (meta data, bug fix, improve test coverage etc).
This is a larger change (new feature, significant refactoring etc). Please use the code review guidelines to add a checklist below to guide the code reviewer.

Code Review Guide

Insert check list here if needed.

CallumNZ

Some changes.

Please add tests to s3_integration_test.go and sqs_integration_test.go, where it's ensured that it gives an error before the asset is created, and then nil error afterwards.

Will need an equivalent set for SNS too.

If the caller doesn't have permission, that will return an error too? Not sure if our testing infrastructure can test for lack of permissions, so maybe add that behaviour to each function documentation.

CallumNZ · 2024-11-27T01:07:37Z

aws/s3/s3.go

@@ -271,6 +271,16 @@ func (s *S3) PutWithMetadata(bucket, key string, object []byte, metadata Meta) e
 	return err
 }

+// checks if the given S3 bucket exists.
+func (s *S3) CheckBucketExists(bucketName string) error {
+	// Check if the bucket exists.


Comment not needed

CallumNZ · 2024-11-27T01:08:05Z

aws/s3/s3.go

@@ -271,6 +271,16 @@ func (s *S3) PutWithMetadata(bucket, key string, object []byte, metadata Meta) e
 	return err
 }

+// checks if the given S3 bucket exists.
+func (s *S3) CheckBucketExists(bucketName string) error {


bucket to be consistent with the others.

CallumNZ · 2024-11-27T01:14:28Z

aws/sqs/sqs.go

+// checks if the given SQS queue exists.
+func (s *SQS) CheckQueueExists(queueName string) error {
+	// Check if the queue exists.
+	_, err := s.client.GetQueueUrl(context.TODO(), &sqs.GetQueueUrlInput{
+		QueueName: aws.String(queueName),
+	})
+
+	return err
+}


I think we usually have the queue URL as an environment variable - is it possible to check from the URL?

CallumNZ · 2024-11-27T19:44:31Z

aws/sqs/sqs_integration_test.go

+	client, err := New()
+
+	// ASSERT
+	assert.Nil(t, err)


Since the test isn't focused on whether client creation works, and subsequent client methods will fail if the client creation failed, this is more appropriate:

client, err := New() require.Nil(t, err, fmt.Sprintf("error creating sqs client: %v", err))

CallumNZ · 2024-11-27T19:48:37Z

aws/sns/sns_integration_test.go

+	client, err := New()
+	ready := client.Ready()
+	// ASSERT
+	assert.Nil(t, err)
+	assert.True(t, ready)


client, err := New() require.Nil(t, err, fmt.Sprintf("error creating sns client: %v", err)) ready := client.Ready() require.True(t, ready, fmt.Sprint("sns client not ready"))

CallumNZ · 2024-11-27T19:52:17Z

aws/sqs/sqs.go

@@ -320,6 +320,18 @@ func (s *SQS) CreateQueue(queueName string, isFifoQueue bool) (string, error) {
 	return aws.ToString(queue.QueueUrl), err
 }

+// checks if the given SQS queue exists.


Suggested change

// checks if the given SQS queue exists.

// CheckQueueExists returns an error if the given queue doesn't exist or cannot be accessed.

CallumNZ · 2024-11-27T19:53:34Z

aws/s3/s3.go

@@ -271,6 +271,15 @@ func (s *S3) PutWithMetadata(bucket, key string, object []byte, metadata Meta) e
 	return err
 }

+// checks if the given S3 bucket exists.


Suggested change

// checks if the given S3 bucket exists.

// CheckBucketExists returns an error if the given bucket doesn't exist or cannot be accessed.

CallumNZ · 2024-11-27T19:54:04Z

aws/sns/sns.go

@@ -116,6 +116,14 @@ func (s *SNS) DeleteTopic(topicArn string) error {
 	return err
 }

+// checks if an SNS topic exists given its ARN.


Suggested change

// checks if an SNS topic exists given its ARN.

// CheckTopicExists returns an error if the given topic doesn't exist or cannot be accessed.

CallumNZ · 2024-11-27T19:56:44Z

On second thought, I think dropping Exists from each function name more clearly indicates what the function does, since it can return an error if the bucket exists but the caller doesn't have the right permissions.

CallumNZ

Just some suggestions to bring inline with Go Docs conventions.

CallumNZ · 2024-11-29T01:06:25Z

aws/s3/s3.go

@@ -271,6 +271,15 @@ func (s *S3) PutWithMetadata(bucket, key string, object []byte, metadata Meta) e
 	return err
 }

+// checks if the given S3 bucket exists and is accessible.


Suggested change

// checks if the given S3 bucket exists and is accessible.

// CheckBucket checks if the given S3 bucket exists and is accessible.

CallumNZ · 2024-11-29T01:07:22Z

aws/sns/sns.go

@@ -116,6 +116,14 @@ func (s *SNS) DeleteTopic(topicArn string) error {
 	return err
 }

+// checks if an SNS topic exists and is accessible given its ARN.


Suggested change

// checks if an SNS topic exists and is accessible given its ARN.

// CheckTopic checks if an SNS topic exists and is accessible given its ARN.

CallumNZ · 2024-11-29T01:07:41Z

aws/sqs/sqs.go

@@ -320,6 +320,18 @@ func (s *SQS) CreateQueue(queueName string, isFifoQueue bool) (string, error) {
 	return aws.ToString(queue.QueueUrl), err
 }

+// checks if the given SQS queue exists and is accessible.


Suggested change

// checks if the given SQS queue exists and is accessible.

// CheckQueue checks if the given SQS queue exists and is accessible.

bpeng requested a review from CallumNZ November 27, 2024 01:03

CallumNZ requested changes Nov 27, 2024

View reviewed changes

bpeng force-pushed the health-check branch 3 times, most recently from 0d06f95 to 5ae4175 Compare November 27, 2024 03:58

CallumNZ requested changes Nov 27, 2024

View reviewed changes

bpeng force-pushed the health-check branch from 5ae4175 to 4a17e70 Compare November 27, 2024 21:31

add functions CheckQueueExists CheckBucketExists

5d1e098

bpeng force-pushed the health-check branch from 4a17e70 to 28165cb Compare November 28, 2024 03:24

CallumNZ reviewed Nov 29, 2024

View reviewed changes

update per review, add integration tests

fcb3308

bpeng force-pushed the health-check branch from 28165cb to fcb3308 Compare November 29, 2024 01:21

CallumNZ approved these changes Nov 29, 2024

View reviewed changes

sue-h-gns merged commit 745247c into main Nov 29, 2024
7 checks passed

sue-h-gns deleted the health-check branch November 29, 2024 02:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add functions CheckQueueExists CheckBucketExists #183

add functions CheckQueueExists CheckBucketExists #183

bpeng commented Nov 27, 2024 •

edited

Loading

CallumNZ left a comment

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ Nov 27, 2024

CallumNZ commented Nov 27, 2024

CallumNZ left a comment

CallumNZ Nov 29, 2024

CallumNZ Nov 29, 2024

CallumNZ Nov 29, 2024

	// checks if the given SQS queue exists.
	// CheckQueueExists returns an error if the given queue doesn't exist or cannot be accessed.

	// checks if the given S3 bucket exists.
	// CheckBucketExists returns an error if the given bucket doesn't exist or cannot be accessed.

	// checks if an SNS topic exists given its ARN.
	// CheckTopicExists returns an error if the given topic doesn't exist or cannot be accessed.

	// checks if the given S3 bucket exists and is accessible.
	// CheckBucket checks if the given S3 bucket exists and is accessible.

	// checks if an SNS topic exists and is accessible given its ARN.
	// CheckTopic checks if an SNS topic exists and is accessible given its ARN.

	// checks if the given SQS queue exists and is accessible.
	// CheckQueue checks if the given SQS queue exists and is accessible.

add functions CheckQueueExists CheckBucketExists #183

add functions CheckQueueExists CheckBucketExists #183

Conversation

bpeng commented Nov 27, 2024 • edited Loading

Proposed Changes

Production Changes

Review

Code Review Guide

CallumNZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CallumNZ commented Nov 27, 2024

CallumNZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpeng commented Nov 27, 2024 •

edited

Loading