Update shadow resource to use FilterTable #2642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
miah
force-pushed
the
miah/2265
branch
3 times, most recently
from
46ce8ef to
257c1ba
Compare
jquick
approved these changes
Feb 20, 2018
chris-rock
reviewed
Feb 20, 2018
lib/resources/shadow.rb
Outdated
|
|
||
| filter.connect(self, :params) | ||
|
|
||
| alias users user |
There was a problem hiding this comment.
Should we add some deprecations here? Based on that improvement, I am not sure what you think users should use.
There was a problem hiding this comment.
Yes, I think we should deprecate the singular forms of user and password. I can update this PR with deprecation notices for those targeting 3.0. I'll update the docs too =)
implementation. Add tests for singluar aliased methods and other minor changes to work with FilterTable output. Coverage is at 100% Signed-off-by: Miah Johnson <[email protected]>
Signed-off-by: Miah Johnson <[email protected]>
chris-rock
approved these changes
Mar 7, 2018
miah
added a commit
that referenced
this pull request
Mar 8, 2018
shadow file. After much thought the deprecations from #2642 were for the wrong methods. Plural method names feel much more natural when working with this resource because you can have more than a single result. Consider a match like `shadow.user(/^www/)`, this could return multiple users, so `shadow.users` feels more natural here. The problem is that the fields we're matching in the shadow file itself are singular. Each entry is for a user, which has a password, and some other fields. A user never has `passwords` in the shadow file, only a `password`. This is made more obvious when you use the `filter` method. When we use this filter: `shadow.filter(min_days: 20, max_days: 30)` we are matching fields in the shadow file and not using our matcher methods. This means that if there is a discrepancy between our matcher methods, and the shadow fields the user could end up confused. Like I did =) This PR changes: Changed matchers to match shadow fields. Updated documentation to reflect changes. Updated tests to reflect changes. Re-add `filter` method, and add a test for it. Renamed variable for FilterTable to be less confusing. Renamed query argument for methods to be consistent. Signed-off-by: Miah Johnson <[email protected]>
miah
added a commit
that referenced
this pull request
Mar 8, 2018
shadow file. After much thought the deprecations from #2642 were for the wrong methods. Plural method names feel much more natural when working with this resource because you can have more than a single result. Consider a match like `shadow.user(/^www/)`, this could return multiple users, so `shadow.users` feels more natural here. The problem is that the fields we're matching in the shadow file itself are singular. Each entry is for a user, which has a password, and some other fields. A user never has `passwords` in the shadow file, only a `password`. This is made more obvious when you use the `filter` method. When we use this filter: `shadow.filter(min_days: 20, max_days: 30)` we are matching fields in the shadow file and not using our matcher methods. This means that if there is a discrepancy between our matcher methods, and the shadow fields the user could end up confused. Like I did =) This PR changes: Changed matchers to match shadow fields. Updated documentation to reflect changes. Updated tests to reflect changes. Re-add `filter` method, and add a test for it. Renamed variable for FilterTable to be less confusing. Renamed query argument for methods to be consistent. Signed-off-by: Miah Johnson <[email protected]>
miah
added a commit
that referenced
this pull request
Mar 8, 2018
shadow file. After much thought the deprecations from #2642 were for the wrong methods. Plural method names feel much more natural when working with this resource because you can have more than a single result. Consider a match like `shadow.user(/^www/)`, this could return multiple users, so `shadow.users` feels more natural here. The problem is that the fields we're matching in the shadow file itself are singular. Each entry is for a user, which has a password, and some other fields. A user never has `passwords` in the shadow file, only a `password`. This is made more obvious when you use the `filter` method. When we use this filter: `shadow.filter(min_days: 20, max_days: 30)` we are matching fields in the shadow file and not using our matcher methods. This means that if there is a discrepancy between our matcher methods, and the shadow fields the user could end up confused. Like I did =) This PR changes: Changed matchers to match shadow fields. Updated documentation to reflect changes. Updated tests to reflect changes. Re-add `filter` method, and add a test for it. Renamed variable for FilterTable to be less confusing. Renamed query argument for methods to be consistent. Cleanup docs based on comments from @jerryaldrichiii Make Rubocop happy <3 Signed-off-by: Miah Johnson <[email protected]>
jquick
pushed a commit
that referenced
this pull request
Mar 8, 2018
…2801) shadow file. After much thought the deprecations from #2642 were for the wrong methods. Plural method names feel much more natural when working with this resource because you can have more than a single result. Consider a match like `shadow.user(/^www/)`, this could return multiple users, so `shadow.users` feels more natural here. The problem is that the fields we're matching in the shadow file itself are singular. Each entry is for a user, which has a password, and some other fields. A user never has `passwords` in the shadow file, only a `password`. This is made more obvious when you use the `filter` method. When we use this filter: `shadow.filter(min_days: 20, max_days: 30)` we are matching fields in the shadow file and not using our matcher methods. This means that if there is a discrepancy between our matcher methods, and the shadow fields the user could end up confused. Like I did =) This PR changes: Changed matchers to match shadow fields. Updated documentation to reflect changes. Updated tests to reflect changes. Re-add `filter` method, and add a test for it. Renamed variable for FilterTable to be less confusing. Renamed query argument for methods to be consistent. Cleanup docs based on comments from @jerryaldrichiii Make Rubocop happy <3 Signed-off-by: Miah Johnson <[email protected]>
clintoncwolfe
pushed a commit
that referenced
this pull request
Jun 19, 2018
* Change shadow resource to use FilterTable rather than custom filter implementation. Add tests for singluar aliased methods and other minor changes to work with FilterTable output. Coverage is at 100% Signed-off-by: Miah Johnson <[email protected]> * merge master Signed-off-by: Miah Johnson <[email protected]>
jquick
pushed a commit
that referenced
this pull request
Jun 19, 2018
…x series (#3154) * Update shadow resource to use FilterTable (#2642) * Change shadow resource to use FilterTable rather than custom filter implementation. Add tests for singluar aliased methods and other minor changes to work with FilterTable output. Coverage is at 100% * Ensure resource has plural properties and singular filter criteria, as it did prior to filtertable-ization Signed-off-by: Clinton Wolfe <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates the shadow resource to use FilterTable rather than a custom filter implementation.
I've also increased test coverage =)
Fixes #2265