-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
libcontainer/specconv/spec_linux: Add support for (no)iversion #1462
Conversation
Part of catching runC up with the spec, which punts valid options to mount(8) [1,2]. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0-rc5/config.md#L68 [2]: opencontainers/runtime-spec#771 Signed-off-by: W. Trevor King <[email protected]>
Did you have to open 4 PRs to add 4 pairs of lines to the same file... 😕 |
On Fri, May 26, 2017 at 12:05:13PM -0700, Aleksa Sarai wrote:
Did you have to open 4 PRs to add 4 pairs of lines to the same file...
I can flatten them into a single PR if you like, but having separate
PRs allows folks to push back on or accept the options with more
granularity. For example, maybe (no)acl is contentious, but
(no)iversion is not. If the PRs stay separate, I'm happy to rebase
whichever land later.
|
Please make them a single PR.
Why not make it 8 PRs so that we have even more granularity? Personally I've always hated In all seriousness, the reason I've had so much trouble keeping track of |
On Fri, May 26, 2017 at 04:14:23PM -0700, Aleksa Sarai wrote:
Please make them a single PR.
Would you like me to roll in #1464 as well? That seems useful
regardless of whether the runC maintainers want to add new mount
options, but I'm happy to include it in #1460 if you'd like to look at
it at the same time as well.
In all seriousness, the reason I've had so much trouble keeping
track of `runtime-spec` PRs is because you keep opening large
numbers of interrelated pull requests. While you might think this
helps, it really doesn't…
I've also been told that my PRs try to do too many things at once and
I should split them up to be more granular [1]. I'm fine going either
way, but it seems best to just guess and then adjust if folks tell me
a particular PR is too fine or coarse. In this case, my initial guess
seems to have been too fine.
[1]: opencontainers/runtime-spec#507 (comment)
|
To be clear, that comment appears to be a separate complaint (conflating formatting changes and semantic changes to a specification document). There is a fairly big difference between a specification document and source code as well. |
On Fri, May 26, 2017 at 04:38:57PM -0700, Aleksa Sarai wrote:
To be clear, that comment appears to be a separate complaint
(conflating formatting changes and semantic changes to a
specification document). There is a fairly big difference between a
specification document and source code as well.
Fair. But the basic idea with decomposing into commits/PRs is to find
orthogonal, rejectable chunks. (no)iversion seemed like something
maintainers might have an opinion on, and the diff here is very small
(and therefore hopefully easy to review) so I thought it deserved its
own PR. If you'd rather wait and form an opinion on a larger #1460
(which I've just updated), that's fine too. If I was supposed to be
able to guess which way would have made it easier for you to form your
opinion, I'm not sure how I should have done that better.
|
Part of catching runC up with the spec, which punts valid options to
mount(8)
. Part of opencontainers/runtime-spec#771.