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

Add S3 Credential datasource and resource #291

Merged
merged 8 commits into from
Aug 26, 2022

Conversation

yvigara
Copy link
Contributor

@yvigara yvigara commented Aug 21, 2022

Add the datasources and resource to create and retrieve S3 Credentials (AccessKeyId and SecretAccessKey) required to access S3 buckets. Fixes #285

@scraly
Copy link
Collaborator

scraly commented Aug 24, 2022

Hi,
thanks for the PR, it's a good addition :)

I have an issue with the first acceptance test:
image

@scraly
Copy link
Collaborator

scraly commented Aug 24, 2022

About acceptance test, is it possible them like this example?
https://github.com/ovh/terraform-provider-ovh/blob/master/ovh/data_cloud_project_database_ip_restrictions_test.go

Thanks

@yvigara
Copy link
Contributor Author

yvigara commented Aug 24, 2022

@scraly thanks a lot for the feedback. All the requested changes should be resolved now.

@yvigara yvigara requested a review from scraly August 24, 2022 21:20
Copy link
Collaborator

@scraly scraly left a comment

Choose a reason for hiding this comment

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

Thanks for the modifications.

Also, regarding the acceptance tests, can you edit them to be consistent like this example please?
https://github.com/ovh/terraform-provider-ovh/blob/master/ovh/data_cloud_project_database_ip_restrictions_test.go

Thanks

ovh/data_cloud_project_user_s3_credential.go Outdated Show resolved Hide resolved
@scraly scraly added the v0.20 label Aug 25, 2022
@yvigara
Copy link
Contributor Author

yvigara commented Aug 25, 2022

Just to be sure, do you mean renaming the config variable to match the test function, keeping it as a simple string and moving the string formatting in the test function?

@scraly
Copy link
Collaborator

scraly commented Aug 25, 2022

I mean moving the hash function to another file, next to the resource file.
Thanks to that in this new helpers file, in the future we can add another helper functions to that resource.

@yvigara
Copy link
Contributor Author

yvigara commented Aug 25, 2022

I'm really sorry I don't see which hash function you are referring to. Apart the naming convention on the config variable and how it's formatted I don't see much difference between these acceptance test functions and the data_cloud_project_database_ip_restrictions_test.go one.
What am I missing?

@scraly
Copy link
Collaborator

scraly commented Aug 26, 2022

Sorry, I reviewed two PRs at the same time so my brain have switched :-D.
About the acceptance test I mean, yes, the const as string and how to pass the config.
Even if a lot of different people contribute to the repo, the goal is to try to keep a coherence when it is possible :).

@yvigara
Copy link
Contributor Author

yvigara commented Aug 26, 2022

Should be all fixed now

@yvigara yvigara requested a review from scraly August 26, 2022 09:15
Copy link
Collaborator

@scraly scraly left a comment

Choose a reason for hiding this comment

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

Perfect, thanks :-)
Acceptance tests are well written now and all the tests are in success.

@scraly scraly merged commit 7a175b8 into ovh:master Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate S3 credentials using Terraform
2 participants