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

Add ACL example #121

Merged
merged 5 commits into from
Sep 19, 2022
Merged

Add ACL example #121

merged 5 commits into from
Sep 19, 2022

Conversation

satyaog
Copy link
Member

@satyaog satyaog commented Jun 1, 2022

Relates to #115

@satyaog satyaog force-pushed the satyaog/aclex branch 9 times, most recently from 4aaf3c3 to f9ef26e Compare June 1, 2022 18:35
@satyaog satyaog marked this pull request as ready for review June 1, 2022 18:36
@satyaog satyaog requested a review from akire0ne as a code owner June 1, 2022 18:36
@satyaog
Copy link
Member Author

satyaog commented Jun 1, 2022

@satyaog
Copy link
Member Author

satyaog commented Jun 1, 2022

@obilaniu , could you quickly look if this is the format you had in mind?

@obilaniu obilaniu self-requested a review June 3, 2022 15:31
Copy link
Contributor

@obilaniu obilaniu left a comment

Choose a reason for hiding this comment

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

It's all good! Thanks for translating from Markdown!

@btravouillon btravouillon self-requested a review June 4, 2022 16:12
Copy link
Collaborator

@btravouillon btravouillon left a comment

Choose a reason for hiding this comment

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

One tiny typo.

Also, while this is very cool, this does not cover all topics of #115. I believe we won't be able to address all topics in a single PR, so I will remove the resolves #115 to avoid case closure.

docs/Userguide_sharing_data.rst Outdated Show resolved Hide resolved
@btravouillon
Copy link
Collaborator

@vict0rsch: Any feedback on these ACL examples?


----

| In order to access a file, all folders from the root (``/``) down to the
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part of the previous section on "searching"? It looks like it's its own different example, but the content seems like it's part of the previous block

@vict0rsch
Copy link
Contributor

vict0rsch commented Jun 6, 2022

Thank you for working on this! I've made a bunch of comments. I think this is still a little raw and rough for beginners to understand completely though.

In particular, I think this PR needs an explicit section on Projects and the ideal workflow for collaborators not to get confused with permissions and people not having access to others' data etc. I'm thinking about a "Good practices for Projects" section with the elements mentioned in #115:

  • group creation request to it-support
  • profs approve
  • provide usernames
  • how to setup ACLs for user groups
    • In particular, what do I do to /network/projects/my_project so that all members of the group have rwx rights to the data (and symmetrically, so that I have rwx rights to their data)
    • In other words, how to setup the directory so that it is systematically, definitively and recursively shared

It's fine to have it on another PR but I think this is a very important topic so maybe it can be dealt with in a single one.

(On that note, can you not make it default that any and all members of a group have rwx on the project's folder /network/projects/my_project so that by default people don't have to do anything with ACLs?)

@btravouillon
Copy link
Collaborator

@obilaniu would you have some time available to update the PR in regards to @vict0rsch comments?

@btravouillon
Copy link
Collaborator

@obilaniu would you have some time available to update the PR in regards to @vict0rsch comments?

@vict0rsch
Copy link
Contributor

@saleml does not know what ACLs are. He'll be a great tester for this PR :)

Copy link
Collaborator

@btravouillon btravouillon 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. 👍

@btravouillon btravouillon merged commit 683bb2b into mila-iqia:master Sep 19, 2022
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