Skip to content
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

Complete the checker logic. #4

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Complete the checker logic. #4

merged 1 commit into from
Jan 24, 2024

Conversation

vprus
Copy link
Collaborator

@vprus vprus commented Jan 23, 2024

The primary changes are:

  • The checks are feature complete
  • The issues are summarized in a text file
  • The recommendations are also saved in terraform format

Incidental changes:

  • Use common code for API calls
  • When processing inventory, use the path from the bucket

The primary changes are:
- The checks are feature complete
- The issues are summarized in a text file
- The recommendations are also saved in terraform format

Incidental changes:
- Use common code for API calls
- When processing inventory, use the path from the bucket
@vprus vprus requested a review from dmgburg January 23, 2024 15:58
Copy link

@PeterIvanov PeterIvanov left a comment

Choose a reason for hiding this comment

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

All my comments at the first glance ended up being rather superficial. I'll try to look again for some more structural suggestions.

tools/storage-advisor/src/api.go Show resolved Hide resolved
tools/storage-advisor/src/main.go Show resolved Hide resolved
tools/storage-advisor/src/main.go Show resolved Hide resolved
tools/storage-advisor/src/main.go Show resolved Hide resolved
tools/storage-advisor/src/inventory.go Show resolved Hide resolved
tools/storage-advisor/src/s3Issues.go Show resolved Hide resolved
tools/storage-advisor/src/s3Issues.go Show resolved Hide resolved

for i := range buckets {
// Get bucket region
r, err := client.GetBucketLocation(context.TODO(), &s3.GetBucketLocationInput{
Bucket: &buckets[i].Name,
})
if err != nil {
buckets[i].Region = err.Error()
buckets[i].Region = "Error" //err.Error()

Choose a reason for hiding this comment

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

Must this remain so?


writeBasicIssues(issues, identity)

if api.IsConfigured() {

Choose a reason for hiding this comment

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

Is it expected by the caller that nothing happens if a token is undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is expected behaviour for now.

fmt.Fprintf(f, "\nS3 issue check was run as %s\n", identity)

if len(issues) == 0 {
fmt.Fprint(f, "\nThere are not issues. You rock!\n")

Choose a reason for hiding this comment

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

Suggested change
fmt.Fprint(f, "\nThere are not issues. You rock!\n")
fmt.Fprint(f, "\nThere are no issues. You rock!\n")

tools/storage-advisor/src/api.go Show resolved Hide resolved
tools/storage-advisor/src/api.go Show resolved Hide resolved
tools/storage-advisor/src/main.go Show resolved Hide resolved
tools/storage-advisor/src/s3Issues.go Show resolved Hide resolved
EncryptionIssue string `json:"encryptionIssue" bson:"encryptionIssue"`
EncryptionIssueDetails string `json:"encryptionIssueDetails" bson:"encryptionIssueDetails"`

UsableInventory string `json:"usableInventory" bson:"usableInventory"`
Copy link

Choose a reason for hiding this comment

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

I'd like to point out that there are unused bson field tags. They do no harm though other than making me question their utility.

return
var ae smithy.APIError
if errors.As(err, &ae) {
color.Red("Could not list buckets: %s: %s", ae.ErrorCode(), ae.ErrorMessage())
Copy link

Choose a reason for hiding this comment

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

I believe fmt.Sprintf("%s: %s", e.ErrorCode(), e.ErrorMessage()) is exactly the way smithy-go-codegen implements smithy.APIError interface.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll need to double-check; here I think I did this explicitly because default one looked too verbose.

}
defer f.Close()

fmt.Fprintf(f, "\nS3 issue check was run as %s\n", identity)
Copy link

Choose a reason for hiding this comment

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

Ideally, errors shouldn't be ignored here.

tools/storage-advisor/src/s3Issues.go Show resolved Hide resolved
tools/storage-advisor/src/s3Issues.go Show resolved Hide resolved
@vprus vprus merged commit 9011f77 into main Jan 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants