-
Notifications
You must be signed in to change notification settings - Fork 362
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
S3 Bucket Policy Support #289
Conversation
bfa74ac
to
e62b392
Compare
@hasheddan I think this PR should be ready for review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @krishchow! A few questions and suggestions below. @muvaf and @negz, how do you feel about AWS taking a string for the policy but us presenting it as structured here to users? Definitely a nice experience from the aspect of implementing referencers within the policy, but could be seen as a deviation from the interface AWS provides to users.
98bd846
to
ccec8ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@muvaf and @negz, how do you feel about AWS taking a string for the policy but us presenting it as structured here to users? Definitely a nice experience from the aspect of implementing referencers within the policy, but could be seen as a deviation from the interface AWS provides to users.
I think it's a good idea if we're able to get the struct and its fields correctly representative of what works in AWS, which could be challenging. One thing I'd want to make sure it works is that the copy-paste of an existing policy should work as-is for a good UX. For example, I have a policy in AWS console and I edit a few small things in my local and copy it to spec of my managed resource. If we keep it as raw string then there is no reason that this wouldn't work but there could be some weird exceptions related to format that we didn't take into account. Is there a formal specification of the JSON schema by AWS that we can refer to?
Other than that, I didn't review the PR very deeply but it looks pretty good, had a few nitpicks caught my eye during my skimming. Thanks @krishchow !
Hey @muvaf! Thanks for the review, these changes look good, I'll make them ASAP. This is the documentation I used.
I can introduce another string field under the spec, should this take priority over the generated body if both are provided, or should this cause an error? |
@krishchow Great, could you refer to this doc in the code as well? If it's a match, copy-paste YAML should just work. I don't think we need an additional raw input field since it seems they pinned down the format to a specific version and we can rely on that. |
@krishchow it appears some of the fields in the documentation you linked are not reflected here such as Not variations and Condition. Are those intentionally excluded? |
fef3791
to
05f3957
Compare
ef43bbd
to
79a9916
Compare
33b3b5a
to
9e59da3
Compare
Signed-off-by: Krish Chowdhary <krish@redhat.com>
69a820b
to
5c16c4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@krishchow thanks for working on this! If you have any examples you have been using and want to follow with a PR to drop one in the examples/
directory that would be awesome!
S3 Bucket Policy Support
S3 Bucket Policy Support
Added (6) resources to DirectConnect group for issue 216
Description of your changes
Fixes #280
This is a semi-complete PR that aims to add support for Bucket Policies which can be provided within the CRD for an S3 Bucket.
Checklist
I have:
make reviewable
to ensure this PR is ready for review.app.yaml
to include any new role permissions.