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

Allow puppetlabs/concat 6.x, puppetlabs/stdlib 6.x #340

Merged
merged 1 commit into from
Jun 9, 2019
Merged

Allow puppetlabs/concat 6.x, puppetlabs/stdlib 6.x #340

merged 1 commit into from
Jun 9, 2019

Conversation

dhoppe
Copy link
Member

@dhoppe dhoppe commented Jun 3, 2019

No description provided.

Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Can you avoid to switch the order of module ? Or any reason for this switch ?

@dhoppe
Copy link
Member Author

dhoppe commented Jun 4, 2019

Can you avoid to switch the order of module ? Or any reason for this switch ?

I like to sort modules in alphabetically order for a better overview. Some modules have a huge list of dependencies.

@Dan33l
Copy link
Member

Dan33l commented Jun 4, 2019

Ah yes ! Good reason for reordering.

What requires to increase the minimal version for concat ?

@dhoppe
Copy link
Member Author

dhoppe commented Jun 4, 2019

@Dan33l Not really. I just used one of @alexjfisher pull requests as boilerplate to introduce some kind of standard for lower boundaries. This version of puppetlabs/concat for example introduced support for Debian 9.

@Dan33l
Copy link
Member

Dan33l commented Jun 4, 2019

@dhoppe i don't know if we have a rule about minimal value requirement.
I my opinion, the lower limit should be moved only when it is required because something does not work.

Since we have acceptance tests on this module, i would prefer to keep unchanged the current minimal version. But i am also fine enough to bump the minimal version.

@dhoppe
Copy link
Member Author

dhoppe commented Jun 4, 2019

@bastelfreak What is your opinion on this topic? I changed the lower boundaries for a lot of modules:

@bastelfreak
Copy link
Member

I dont care about the ordering, I just try to keep the diff small and I am too lazy reorder dependencies.


the minimal version should only be bumped if this there is a reason for it (puppet5/6 support, debian 9 support....). But in that case we need to update the PR title and mark it as backwards-incompatible.

@alexjfisher
Copy link
Member

alexjfisher commented Jun 5, 2019

I'm not in favour of making major releases over something so trivial. We've already dropped puppet 4 support (with a major version bump) and concat doesn't support puppet 5 until version 4.1.0. As such, you could consider bumping of the minimum concat version as a bugfix/cosmetic change.

@dhoppe
Copy link
Member Author

dhoppe commented Jun 6, 2019

@alexjfisher I agree, but someone told me that I need to label the pull requests as backwards-incompatible.

I have made ~20 pull requests of this kind and it is no fun to change them every time someone makes a statement. :(

Especially because I used one of your pull requests as base and you did not label the pull request as backwards-incompatible.

@bastelfreak
Copy link
Member

I agree with @alexjfisher here. I think the bug label makes more sense here instead of backwards-incompatible. Anybody has a different opinion?

@Dan33l
Copy link
Member

Dan33l commented Jun 9, 2019

My primary trouble is we mix two changes:

  • a maintenance update
  • a bugfix that i was seeing like a breaking change.

So finally, i am fine enough if we change the title of PR and add bug label.

@alexjfisher alexjfisher merged commit 433ba31 into voxpupuli:master Jun 9, 2019
@bastelfreak bastelfreak added the bug Something isn't working label Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants