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

Add a Stdlib::CreateResources type #1267

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Sep 27, 2022

It is very common to have a parameter which creates resources, either via create_resources() or via iteration.

@ekohl ekohl requested a review from a team as a code owner September 27, 2022 09:41
bastelfreak
bastelfreak previously approved these changes Sep 27, 2022
@pmcmaw pmcmaw added the feature label Sep 27, 2022
@david22swan
Copy link
Member

@ekohl This look's good to me.
It need's a rebase before I can merge it though.

@chelnak
Copy link
Contributor

chelnak commented Oct 3, 2022

I'm a fan of types being singular nouns. I wonder if calling this type 'resource' would be too ambiguous?

It is very common to have a parameter which creates resources, either
via `create_resources()` or via iteration.
@ekohl
Copy link
Collaborator Author

ekohl commented Oct 3, 2022

I'm a fan of types being singular nouns. I wonder if calling this type 'resource' would be too ambiguous?

This describes multiple resources. You can't get away from it IMHO. For example, you also have ensure_resource and ensure_resources. If you were to define types for that, you'd also have one singular and one plural.

Technically you have this:

type Stdlib::ResourceAttribute = Any
type Stdlib::ResourceAttributes = Hash[String[1], Stdlib::ResourceAttribute]
type Stdlib::CreateResources = Hash[String[1], Stdlib::ResourceAttributes]

But that's really verbose.

Also, you could also narrow down the title to a Pattern that's much smaller, but I'm not sure how much you'd win.

@chelnak
Copy link
Contributor

chelnak commented Oct 3, 2022

Yeah fair enough. I can see your point here. 👍

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

LGTM

@david22swan david22swan merged commit 909061c into puppetlabs:main Oct 4, 2022
@david22swan
Copy link
Member

Thank's for getting the rebase done so fast :)

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.

6 participants