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

(MODULES-8909) Add type-aliases and auto-loading. #214

Merged
merged 14 commits into from
Apr 29, 2019

Conversation

pillarsdotnet
Copy link
Contributor

@pillarsdotnet pillarsdotnet commented Apr 9, 2019

Adds defined type-aliases for some parameters to the Accounts::User resource type and accounts auto-loading per MODULES-8909.

Copy link

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Let me detail the reason for the downvote.

Optional[Foo] vs. Foo hiding it is Optional

A quick glance at the parameters list does not help in figuring out which parameters are required and which parameters are optional. I feel like defined types should not not say their value is optional, and Optional[] should be put when using the type when applicable.

That way, those defined types can be reused more easily if it makes sense in another module where it is required.

Boolean $system = false vs. Accounts::User::System $system = false

IMO this kind of change does not improve readability, and makes it more complex to understand that this parameter expect a Booolean, especially if no default value was set.

Some parameters do benefit from aliases though, mostly those using regular expressions, e.g. Pattern[/^\/$|^\/.*[^\/]$/] vs. Stdlib::Unixpath, but I feel it is better to make the possible values obvious instead of hiding them behind a custom type, e.g. Pattern[/^absent$|^\d{4}-\d{2}-\d{2}$/] (makes me think) vs. Accounts::User::Expiry (makes me think hard the first time I see it, straightforward the next times until I forgot about this) vs. Variant[Enum['absent'], ISO8601Date] (makes me think a little every time).

types/users.pp Outdated Show resolved Hide resolved
manifests/user.pp Outdated Show resolved Hide resolved
@pillarsdotnet
Copy link
Contributor Author

@smortex -- Thanks for the review. I have tried to follow all your suggestions.

Ready for another look?

Copy link

@smortex smortex left a comment

Choose a reason for hiding this comment

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

IMO the interface looks a lot friendlier to use, thanks!

types/user/hash.pp Outdated Show resolved Hide resolved
types/user/iterations.pp Outdated Show resolved Hide resolved
@smortex
Copy link

smortex commented Apr 12, 2019

That looks great, @pillarsdotnet.

I am not a collaborator to this project (I don't recall how I reach this PR, I just wanted to give feedback), so you'll have to wait for some project owner to review and merge this 😉

@eimlav
Copy link
Contributor

eimlav commented Apr 15, 2019

Hi @pillarsdotnet . Thanks for adding these new types. They should be a great new feature for the module! Would it be possible to update your commit message to include your ticket number (see CONTRIBUTING.md). Since there are a number of new types added, these all require unit tests as well as Puppet Strings documentation. If you are looking for examples of how to do testing on type aliases, see stdlib testing.

Let me know if you have any questions. Cheers!

@eimlav
Copy link
Contributor

eimlav commented Apr 16, 2019

@pillarsdotnet Great stuff! All this needs now is a few README updates to document the new types. Something along the lines of this should suffice. After that it should be ready to go :)

@pillarsdotnet
Copy link
Contributor Author

pillarsdotnet commented Apr 19, 2019

Passes all tests and ready for final review by @eimlav and/or @DavidS

Copy link
Contributor

@eimlav eimlav left a comment

Choose a reason for hiding this comment

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

Some minor changes. Also ensure to run Puppet Strings to regenerate the REFERENCE.md. See here for more info on this.

manifests/init.pp Outdated Show resolved Hide resolved
manifests/init.pp Outdated Show resolved Hide resolved
manifests/manage_keys.pp Outdated Show resolved Hide resolved
manifests/manage_keys.pp Outdated Show resolved Hide resolved
manifests/manage_keys.pp Outdated Show resolved Hide resolved
spec/spec_helper_acceptance.rb Outdated Show resolved Hide resolved
spec/spec_helper_acceptance.rb Outdated Show resolved Hide resolved
spec/spec_helper_acceptance.rb Outdated Show resolved Hide resolved
spec/spec_helper_acceptance.rb Outdated Show resolved Hide resolved
spec/spec_helper_acceptance.rb Outdated Show resolved Hide resolved
@tphoney
Copy link
Contributor

tphoney commented Apr 29, 2019

@pillarsdotnet phenomenal work. Best PR i have seen in a while !!

@eimlav
Copy link
Contributor

eimlav commented Apr 29, 2019

Adhoc ran successfully:

See here for results Screen Shot 2019-04-29 at 11 13 02
Fantasic work @pillarsdotnet ! I'm happy to merge this now if you are @tphoney ?

@tphoney tphoney merged commit 5a6eb97 into puppetlabs:master Apr 29, 2019
@tphoney
Copy link
Contributor

tphoney commented Apr 29, 2019

great work !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants