You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by tequalsme <gi...@git.apache.org> on 2016/08/05 21:32:59 UTC
[GitHub] nifi pull request #801: NIFI-2507 Added Canned ACL support to PutS3Object
GitHub user tequalsme opened a pull request:
https://github.com/apache/nifi/pull/801
NIFI-2507 Added Canned ACL support to PutS3Object
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tequalsme/incubator-nifi NIFI-2507
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/801.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #801
----
commit 4e6dc857f462923d3ebc018d3d35e8c205d1e249
Author: Tim Reardon <te...@gmail.com>
Date: 2016-08-05T21:06:38Z
NIFI-2507 Added Canned ACL support to PutS3Object
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #801: NIFI-2507 Added Canned ACL support to PutS3Object
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/801
@tequalsme , would you please update the integration test `ITPutS3Object.testGetPropertyDescriptors()` (from ITPutS3Object.java:314)? The test currently fails on line 317 after finding 26 property descriptors on PutS3Object rather than the expected 25. You might also add a check for the CANNED_ACL descriptor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #801: NIFI-2507 Added Canned ACL support to PutS3Object
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/801
Thanks for the update, @tequalsme, I verified:
* Property descriptor name/displayName updated
* Reformated createCannedACL is appreciated, thanks
* Exception error message from bad canned ACL names is reasonably intelligible
* Updated integration test ITPutS3Object.testGetPropertyDescriptors is passing
* Comments are a nice enhancement, and the generated Javadoc looks good
I will squash, rebase, and merge shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #801: NIFI-2507 Added Canned ACL support to PutS3Object
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/801#discussion_r73953981
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/AbstractS3Processor.java ---
@@ -184,35 +194,59 @@ protected String getUrlForObject(final String bucket, final String key) {
}
protected final AccessControlList createACL(final ProcessContext context, final FlowFile flowFile) {
- final AccessControlList acl = new AccessControlList();
+ // lazy-initialize ACL, as it should not be used if no properties were specified
+ AccessControlList acl = null;
final String ownerId = context.getProperty(OWNER).evaluateAttributeExpressions(flowFile).getValue();
if (!StringUtils.isEmpty(ownerId)) {
final Owner owner = new Owner();
owner.setId(ownerId);
+ if (acl == null) {
+ acl = new AccessControlList();
+ }
acl.setOwner(owner);
}
for (final Grantee grantee : createGrantees(context.getProperty(FULL_CONTROL_USER_LIST).evaluateAttributeExpressions(flowFile).getValue())) {
+ if (acl == null) {
+ acl = new AccessControlList();
+ }
acl.grantPermission(grantee, Permission.FullControl);
}
for (final Grantee grantee : createGrantees(context.getProperty(READ_USER_LIST).evaluateAttributeExpressions(flowFile).getValue())) {
+ if (acl == null) {
+ acl = new AccessControlList();
+ }
acl.grantPermission(grantee, Permission.Read);
}
for (final Grantee grantee : createGrantees(context.getProperty(WRITE_USER_LIST).evaluateAttributeExpressions(flowFile).getValue())) {
+ if (acl == null) {
+ acl = new AccessControlList();
+ }
acl.grantPermission(grantee, Permission.Write);
}
for (final Grantee grantee : createGrantees(context.getProperty(READ_ACL_LIST).evaluateAttributeExpressions(flowFile).getValue())) {
+ if (acl == null) {
+ acl = new AccessControlList();
+ }
acl.grantPermission(grantee, Permission.ReadAcp);
}
for (final Grantee grantee : createGrantees(context.getProperty(WRITE_ACL_LIST).evaluateAttributeExpressions(flowFile).getValue())) {
+ if (acl == null) {
+ acl = new AccessControlList();
+ }
acl.grantPermission(grantee, Permission.WriteAcp);
}
return acl;
}
+
+ protected final CannedAccessControlList createCannedACL(final ProcessContext context, final FlowFile flowFile) {
+ final String cannedAcl = context.getProperty(CANNED_ACL).evaluateAttributeExpressions(flowFile).getValue();
+ return (!StringUtils.isEmpty(cannedAcl)) ? CannedAccessControlList.valueOf(cannedAcl) : null;
--- End diff --
How about breaking this out into a few simpler if/else lines?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #801: NIFI-2507 Added Canned ACL support to PutS3Object
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on a diff in the pull request:
https://github.com/apache/nifi/pull/801#discussion_r73953158
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/AbstractS3Processor.java ---
@@ -82,6 +83,15 @@
.description("A comma-separated list of Amazon User ID's or E-mail addresses that specifies who should have permissions to change the Access Control List for an object")
.defaultValue("${s3.permissions.writeacl.users}")
.build();
+ public static final PropertyDescriptor CANNED_ACL = new PropertyDescriptor.Builder()
+ .name("Canned ACL")
--- End diff --
Would you please use `name()` as a property key ("canned-acl") and use `displayName()` for the UI ("Canned ACL")? I know you matched the existing code, but we're trying to move to the name+displayName standard.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #801: NIFI-2507 Added Canned ACL support to PutS3Object
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/801
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #801: NIFI-2507 Added Canned ACL support to PutS3Object
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/801
@tequalsme, thanks for contributing this, I think it's a great idea. I will review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---