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

Inomurko/iam #70

Merged
merged 8 commits into from
Aug 1, 2024
Merged

Inomurko/iam #70

merged 8 commits into from
Aug 1, 2024

Conversation

InoMurko
Copy link
Contributor

@InoMurko InoMurko commented Jul 30, 2024

Pull Request Template

Fixes Issue

#69

Fixes #

Changes proposed

  • Adds a PATH within s3 bucket variable
  • Adds the ability to authenticate iam or static

Screenshots (Optional)

Note to reviewers

Copy link
Collaborator

@ethenotethan ethenotethan left a comment

Choose a reason for hiding this comment

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

LGTM - the changes are pretty lightweight. Left some comments but all knits 🙏🏻

server/config.go Outdated
Comment on lines 169 to 176
func toS3CredentialType(s string) store.S3CredentialType {
if s == string(store.S3CredentialStatic) {
return store.S3CredentialStatic
} else if s == string(store.S3CredentialIAM) {
return store.S3CredentialIAM
}
return store.S3CredentialUnknown
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

knit - switch might be more idiomatic

Copy link
Collaborator

Choose a reason for hiding this comment

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

also it might make more sense for this function to live in store/s3.go

}

type S3Store struct {
cfg S3Config
client *minio.Client
stats *Stats
stats *Stats
}

func NewS3(cfg S3Config) (*S3Store, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

knit - this function could be refactored to support less duplication if we were to add a receiver to S3 config for generating the client credentials func (cfg *Config) Credentials() minio.credentials and only have a single instillation of the client in the s3 constructor

@ethenotethan ethenotethan merged commit 940f074 into Layr-Labs:main Aug 1, 2024
8 of 10 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.

2 participants