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

feat: initial implementation #1

Merged
merged 8 commits into from
Jun 14, 2022
Merged

feat: initial implementation #1

merged 8 commits into from
Jun 14, 2022

Conversation

jsmvaldivia
Copy link
Contributor

New action to fetch AWS appconfig configuration

@jsmvaldivia jsmvaldivia self-assigned this Jun 8, 2022
@jsmvaldivia jsmvaldivia requested a review from jcrqr June 8, 2022 15:39
@jsmvaldivia jsmvaldivia added documentation Improvements or additions to documentation enhancement New feature or request labels Jun 8, 2022
@jsmvaldivia jsmvaldivia requested a review from cb-pt-sanmar June 8, 2022 15:40
Copy link
Contributor

@jcrqr jcrqr left a comment

Choose a reason for hiding this comment

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

Apart from the comments, don't forget to add at least a CODEOWNERS and LICENSE files.

The README.md file should also mention the license at the bottom.

Apart from that, great job! :shipit:

README.md Outdated
- AWS AppConfig Application should exist already
- AWS Appconfig Application Configuration profile should exist already
- AWS AppConfig Environment should exist already

Copy link
Contributor

Choose a reason for hiding this comment

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

Space between list items

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

README.md Outdated
Comment on lines 25 to 26


Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newlines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

aws/aws.go Outdated
return string(output.Configuration), nil
}

func awsSdkConfig(region string) (aws.Config, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a context.Context argument here and pass the same context used for the other AWS functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds reasonable

aws/aws.go Outdated
"github.com/aws/aws-sdk-go-v2/service/appconfigdata"
)

func GetConfig(appName, profileName, env, region string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a context.Context here that can be passed by the calling code and re-used in every AWS function (better if we ever decide to do something we the context we can do it in the main file instead of having to update multiple places).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds reasonable

@jsmvaldivia jsmvaldivia requested a review from jcrqr June 9, 2022 14:50
Copy link
Contributor

@jcrqr jcrqr left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏼

@jsmvaldivia jsmvaldivia merged commit 0123a9a into main Jun 14, 2022
@jsmvaldivia jsmvaldivia deleted the feat/init branch June 14, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants