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 an Ensure type #749

Closed
wants to merge 1 commit into from
Closed

Conversation

npwalker
Copy link
Contributor

No description provided.

@ekohl
Copy link
Collaborator

ekohl commented Mar 29, 2017

The problem with this is that in some cases the ensure is overloaded. Think of package where it can also be installed or a specific version. I wonder if this should be EnsurePresence would be a better name. Yay bikeshedding ;)

@npwalker
Copy link
Contributor Author

@ekohl I think the resolution to that is to have a Package_ensure type that allows for latest and versions. But I think it's better to validate against the smallest set of values instead of anything that ensure will accept. For example, ensure accepts true and false but they shouldn't be used.

@ekohl
Copy link
Collaborator

ekohl commented Mar 29, 2017

@npwalker I agree, but my main worry is that people start using Ensure on package and accidentally break things. Another name could be BaseEnsure. Then a PackageEnsure could extend this: Enum[BaseEnsure, 'installed', Pattern['\d+(\.\d+)*']].

@alexjfisher
Copy link
Collaborator

@ekohl Further complicated by some package providers (ie puppet_gem) allowing things like ~> 1.0

@tphoney
Copy link
Contributor

tphoney commented Sep 25, 2017

Hi everyone, I think there are a number of issues raised here which prevent this from being merged as is. I am closing this PR for now. If you can address the issues outlined above please feel free to create a new PR. Many thanks for the work and reviewing people have done

@tphoney tphoney closed this Sep 25, 2017
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.

None yet

6 participants