Skip to content

[CURATOR-362] Use provided ACL when creating parent directories#223

Merged
asfgit merged 1 commit intoapache:masterfrom
szekizoli:CURATOR-362
Jul 19, 2017
Merged

[CURATOR-362] Use provided ACL when creating parent directories#223
asfgit merged 1 commit intoapache:masterfrom
szekizoli:CURATOR-362

Conversation

@Zoltan-Szekeres
Copy link
Copy Markdown
Contributor

No description provided.

@Randgalt
Copy link
Copy Markdown
Member

I'm not certain this is the correct behavior. The API contract implies only that the ACL is set for the node being created. The ACL provider's job is to manage any other nodes. This might be a breaking change for existing clients. I'm -1 on this. I'm curious what other committers think.

@cammckenzie
Copy link
Copy Markdown
Contributor

I agree that this is now a 'feature' that the parents don't have the ACL set. Perhaps we need to introduce another option to set the ACL on the parents?

@Randgalt
Copy link
Copy Markdown
Member

We can add a new method to ACLable:

public interface ACLable<T>
{
    public T withACL(List<ACL> aclList);

    public T withACL(List<ACL> aclList, boolean applyToParents);
}

@szekizoli
Copy link
Copy Markdown
Contributor

I agree that introducing a new behavior is better through a new method. If everyone likes this solution, I'll implement it.

@cammckenzie
Copy link
Copy Markdown
Contributor

Sounds good to me, thanks @szekizoli

@szekizoli szekizoli force-pushed the CURATOR-362 branch 2 times, most recently from c2873af to e9b3ce8 Compare May 26, 2017 23:35
Copy link
Copy Markdown
Contributor

@cammckenzie cammckenzie left a comment

Choose a reason for hiding this comment

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

Looks good to me, I just wonder if we need to provide the ACL functionality for the exists builder also given it contains creating parent containers functionality?

if ( createParentContainersIfNeeded || createParentsIfNeeded )
{
CreateBuilderImpl.backgroundCreateParentsThenNode(client, operationAndData, operationAndData.getData(), backgrounding, createParentContainersIfNeeded);
CreateBuilderImpl.backgroundCreateParentsThenNode(client, operationAndData, operationAndData.getData(), backgrounding, client.getAclProvider(), createParentContainersIfNeeded);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should the ExistsBuilder have the ability to set the ACL provider for the parent containers as well, given that the we provide the option of creating parent containers?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think yes, it'd better if one can set the ACL as well with the option of creating parents, because the ability to set the ACL when creating a node is the expected behaviour.

@Randgalt
Copy link
Copy Markdown
Member

What's the status of this PR? I'd like to get a release out.

@szekizoli
Copy link
Copy Markdown
Contributor

I can prepare an update with the ability to add an ACL provider on the ExistsBuilder if createParentNodesIfNeeded() is called, if you think that's required for this change. It seems more consistent to me with having the option to set the ACL.

@Randgalt
Copy link
Copy Markdown
Member

Yes, please. If you do it today it can be in the next release.

@szekizoli
Copy link
Copy Markdown
Contributor

I've updated the PR. I've decided to add the withACL(List, boolean) method to a new interface called ParentACLable, because it seemed to me that the applyToParents doesn't make sense in all case. For example, in the case of an ExistsBuilder, where only the parent nodes are created. But I have a version with the interface not split as well.

CURATOR-362 - Cover case when no ACL list is provided to CreateBuilder

CURATOR-362 - Use provided ACL for creating parents in background operation

CURATOR-362 - Use provided ACL for creating parents in background operation
@asfgit asfgit merged commit 7e611bd into apache:master Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants