Cross Account - sourceArn #19
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
blueimp
reviewed
Nov 19, 2021
| @@ -29,6 +29,9 @@ var ( | |||
| user = flag.String("u", "", "Authentication username") | |||
| allowFrom = flag.String("l", "", "Allowed sender emails regular expression") | |||
| denyTo = flag.String("d", "", "Denied recipient emails regular expression") | |||
| sourceArn = flag.String("f", "", "The SourceARN (when using with cross accounts) ") | |||
| fromArn = flag.String("o", "", "The FromARN (when using with cross accounts)") | |||
| rPathArn = flag.String("p", "", "The ReturnPathARN (when using with cross accounts)") | |||
There was a problem hiding this comment.
For consistency, we should call this variable returnPathArn.
| @@ -29,6 +29,9 @@ var ( | |||
| user = flag.String("u", "", "Authentication username") | |||
| allowFrom = flag.String("l", "", "Allowed sender emails regular expression") | |||
| denyTo = flag.String("d", "", "Denied recipient emails regular expression") | |||
| sourceArn = flag.String("f", "", "The SourceARN (when using with cross accounts) ") | |||
| fromArn = flag.String("o", "", "The FromARN (when using with cross accounts)") | |||
There was a problem hiding this comment.
Would switch the flags for sourceArn and fromArn, since f is better associated with from.
| @@ -29,6 +29,9 @@ var ( | |||
| user = flag.String("u", "", "Authentication username") | |||
| allowFrom = flag.String("l", "", "Allowed sender emails regular expression") | |||
| denyTo = flag.String("d", "", "Denied recipient emails regular expression") | |||
| sourceArn = flag.String("f", "", "The SourceARN (when using with cross accounts) ") | |||
There was a problem hiding this comment.
Would change the description of all three flags to the following (for consistency with setName):
- Amazon SES SourceARN
- Amazon SES FromARN
- Amazon SES ReturnPathARN
| } | ||
| if *rPathArn == "" { | ||
| rPathArn = nil | ||
| } |
There was a problem hiding this comment.
Could you elaborate on why you're setting the variables to nil here?
| @@ -16,6 +16,9 @@ type Client struct { | |||
| setName *string | |||
| allowFromRegExp *regexp.Regexp | |||
| denyToRegExp *regexp.Regexp | |||
| sourceArn *string | |||
| fromArn *string | |||
| returnPathArn *string | |||
There was a problem hiding this comment.
I think it might be better to create a struct called arns that consists of those three arns and add that as member to this struct. Makes handling the arguments for a lot of the function calls a bit simpler.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Beginner with golang, I try to fixes #16 by implementing
SourceArn,FromArnandReturnPathArnAbout adding some initializations in the
configurefunction in main.go (e58ac12) , I did so because when you provide only SourceArn without FromArn and ReturnPathArn, it seems that it don't work like the documentation says