Skip to content
This repository was archived by the owner on Apr 13, 2022. It is now read-only.

Clarify container deletion, fix #172 - #177

Merged
michielbdejong merged 5 commits into
masterfrom
container-delete
Jun 18, 2019
Merged

Clarify container deletion, fix #172#177
michielbdejong merged 5 commits into
masterfrom
container-delete

Conversation

@michielbdejong

@michielbdejong michielbdejong commented May 28, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@michielbdejong

michielbdejong commented May 28, 2019

Copy link
Copy Markdown
Contributor Author

@fabiancook can we merge this, making the current spec like rmdir, and then you can open an issue for your proposal to make it like rm -rf?

@fabiancook

fabiancook commented May 28, 2019 •
edited
Loading

Copy link
Copy Markdown

This seems like it defeats the purpose of the discussion we're having on the issue. By leaving this off, there is no spec change for rm -rf, it's a clarification, where this is a restriction?

All but one implementation referenced would be not up to spec given this PR

@michielbdejong

michielbdejong commented May 28, 2019

Copy link
Copy Markdown
Contributor Author

OK so then we disagree not only on what the desired situation is but also on what the current situation is. :) Let's discuss that on #172.

This was referenced May 29, 2019
Open
Closed
acoburn
acoburn reviewed May 30, 2019
View reviewed changes

@acoburn acoburn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am basically 👍 on this, modulo my comment about "member resources"

Comment thread api-rest.md Outdated
```

### Deleting containers
A container can only be deleted if it contains neither member resources nor ACL documents.

@acoburn acoburn May 30, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe you mean "contained" resources here. "Member resources" has a very different meaning in LDP.

@michielbdejong michielbdejong mentioned this pull request May 31, 2019
Closed
RubenVerborgh
RubenVerborgh suggested changes Jun 13, 2019
View reviewed changes

@RubenVerborgh RubenVerborgh left a comment

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.

Phrasing is not correct; the term "member resources" is incorrect, and they are seemingly presented as disjoint from ACL resources, which is also not correct.

@michielbdejong

michielbdejong commented Jun 17, 2019

Copy link
Copy Markdown
Contributor Author

@RubenVerborgh @acoburn I think this should address both your comments, can you re-review?

kjetilk
kjetilk approved these changes Jun 18, 2019
View reviewed changes

@kjetilk kjetilk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indeed!

@michielbdejong

michielbdejong commented Jun 18, 2019

Copy link
Copy Markdown
Contributor Author

great, thanks!

@michielbdejong
michielbdejong merged commit b941ff7 into master Jun 18, 2019
@michielbdejong
michielbdejong deleted the container-delete branch June 18, 2019 09:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers
@acoburn acoburn acoburn left review comments
@kjetilk kjetilk kjetilk approved these changes
@RubenVerborgh RubenVerborgh RubenVerborgh approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@michielbdejong @fabiancook @kjetilk @RubenVerborgh @acoburn