Skip to content

Add LinuxMint to operatingsystem and osfamily facts#453

Closed
ccsalvesen wants to merge 5 commits intopuppetlabs:masterfrom
ccsalvesen:patch-1
Closed

Add LinuxMint to operatingsystem and osfamily facts#453
ccsalvesen wants to merge 5 commits intopuppetlabs:masterfrom
ccsalvesen:patch-1

Conversation

@ccsalvesen
Copy link

No description provided.

Choose a reason for hiding this comment

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

Is this meaningful for Linux Mint? This seems like it might be an unintentional duplication of the Ubuntu fact.

Copy link
Author

Choose a reason for hiding this comment

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

The comment is not relevant, it's a sloppy copy-paste job. The rest is very relevant.

@adrienthebo
Copy link

Thank you very much for this contribution!

It looks like there aren't any tests for these changes. To make sure that the changes added here don't regress in the future, new changes should have tests that verify the new behavior introduced here. For reference, the existing tests for the operatingsystemrelease fact (https://github.com/puppetlabs/facter/blob/master/spec/unit/operatingsystemrelease_spec.rb#L17) could be extended to test for the Linux Mint version file, and the tests for the Ubuntu operatingsystem fact (https://github.com/puppetlabs/facter/blob/master/spec/unit/operatingsystem_spec.rb#L85) could be copied and changed for Linux Mint.

I've also added some inline comments on things that could use clarification or improvement.

Also, I noticed that we don't have a contributor license agreement on file. We'll
need you to electronically sign the CLA per the instructions in CONTRIBUTING.md.

@puppetcla
Copy link

CLA Signed by ccsalvesen on 2013-06-05 13:46:39 -0700

@ccsalvesen
Copy link
Author

Thank you for your feedback. I've signed the CLA.

I've got extremely limited experience with ruby, but I'll try to add some test-cases.

@adrienthebo
Copy link

If you would like help with tests I'm very happy to lend a hand. IRC would be one of the best ways to get ahold of me. I'm generally available on irc.freenode.net from 9:00 AM - 5:00 PM in #puppet and #puppet-dev, and my nickname is finch.

@adrienthebo
Copy link

I noticed this hasn't seen any activity in about a week. Is there a chance you'll be able to work on this in the coming week? No pressure, just checking in.

@adrienthebo
Copy link

It looks like this pull request hasn't seen any activity in about two weeks. Is this something that you're still able to work on? We're happy to keep this pull request open if there's forward movement, and we'll leave it open for another two weeks from now. If there isn't any activity by then we may have to close this pull request due to lack of activity.

Closing the pull request wouldn't mean we don't consider this change valuable. However we try to keep all open pull requests moving towards completion, and closing stale pull requests help us focus on more active pull requests.

If you have any questions or concerns, please don't hesitate to ping us in #puppet-dev on irc.freenode.net.

@nibalizer
Copy link

Hi @adrienthebo @ccsalvesen,

I believe I have made some tests for this, not too sure about how to tell github though.

https://github.com/nibalizer/facter/tree/patch-1-add-tests

@adrienthebo
Copy link

@nibalizer I tried cherry-picking 953b81c onto this pull request but I got a merge conflict. The head of this pull request is d35d70d whereas the last commit from @ccsalvesen is 7d43efc ; did you rebase your branch before adding your commits?

@nibalizer
Copy link

@adrienthebo Yes I cloned from this PR then git pull --rebase'd against plabs/facter/HEAD. I had to do this since tests for these facts changed between when PR was written (or maybe when @ccsalvesen forked?) and when you made suggestions for how to add tests. Would it be helpful for me to just PR from my patch-1-add-tests branch and we can close this? That branch preserves the work @ccsalvesen did and attributes him accordingly.

@adrienthebo
Copy link

summary: merged into master in 17ece7c; this should be released in 2.0.0. Thanks for the contribution @ccsalvesen and @nibalizer !

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants