(PUP-2609) Windows error on use source permissions #3226
Conversation
MikaelSmith
commented
Oct 22, 2014
MikaelSmith
commented
- (PUP-2609) Make copying permissions on Win an error
- (PUP-2609) Update Metadata default permission
MikaelSmith
changed the title
|
@joshcooper @Iristyle @ferventcoder One of you mind taking a close look at this to make sure I've got changes to the ACL tests right? |
|
CLA signed by all contributors. |
|
@MikaelSmith I'll take a look at it. |
| @@ -27,7 +27,7 @@ class MetaStat | |||
|
|
|||
| def initialize(stat, source_permissions = nil) | |||
| @stat = stat | |||
| @source_permissions_ignore = source_permissions == :ignore | |||
| @source_permissions_ignore = (!source_permissions or source_permissions == :ignore) | |||
There was a problem hiding this comment.
Since this isn't flow control, we should use || here.
|
@ferventcoder Do you care about expect {}.to vs expect {}.should? |
|
RSpec 3 deprecated |
Copying permissions on Windows are problematic, so it isn't done. Change the deprecation warning about source_permissions => use/use_when_creating to an error.
MikaelSmith
force-pushed
the
task/master/PUP-2609-Windows-error-on-use-source-permissions
branch
from
9b45ff2
to
7686e53
Compare
| expect { win_stat.owner }.to raise_error(ArgumentError) | ||
| expect { win_stat.group }.to raise_error(ArgumentError) | ||
| expect { win_stat.mode }.to raise_error(ArgumentError) | ||
| expect { Puppet::FileServing::Metadata::WindowsStat.new(stat, path, :use) }.to raise_error(ArgumentError) |
There was a problem hiding this comment.
Is this the best error that can be raised? I'd prefer to scope out a little of the error text and validate it's the error we are looking for. Seeing what you did in source_spec and above this line should be emulated here. Perhaps it is not possible though. That's fine. If it can be changed, this would be the only thing holding up a merge.
There was a problem hiding this comment.
Should be able to be more specific. I'll update it.
Update Metadata's default permission model to ignore source permissions, and remove unreachable code. Update spec tests for new behavior.
MikaelSmith
force-pushed
the
task/master/PUP-2609-Windows-error-on-use-source-permissions
branch
from
7686e53
to
02bf973
Compare
| Puppet::Util::Windows::Security.stubs(:get_owner).with(path).raises(invalid_error) | ||
| Puppet::Util::Windows::Security.stubs(:get_group).with(path).raises(invalid_error) | ||
| Puppet::Util::Windows::Security.stubs(:get_mode).with(path).raises(invalid_error) | ||
| Puppet::Util::Windows::Security.stubs(:get_owner).with(path, :use).raises(invalid_error) |
There was a problem hiding this comment.
These stubs will never be called. Is it worth stubbing them out? Same question about the whole previous test.
There was a problem hiding this comment.
What used to call them? A test that was removed? Or have they just been orphaned?
There was a problem hiding this comment.
These look like guards to me to ensure that those methods are not called as an artifact of testing.
There was a problem hiding this comment.
The calls to retrieve win_stat.owner/group/mode would call them unless source_permissions=:ignore was passed in. That's no longer supported, but they still serve as guards.
Edit: meant :ignore, not :use.
There was a problem hiding this comment.
So ignore would end up calling them?
|
I'm good with this as it is now. |
…s-error-on-use-source-permissions (PUP-2609) Windows error on use source permissions
MikaelSmith
deleted the
task/master/PUP-2609-Windows-error-on-use-source-permissions
branch
