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

Updated default.rb to run only on Debian #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Updated default.rb to run only on Debian #16

wants to merge 2 commits into from

Conversation

axelrtgs
Copy link

@axelrtgs axelrtgs commented Dec 22, 2016

the cookbook runs when included in a role run list consisting of mixed nodes (I.E. Ubuntu and windows).
Expected behavior would be to test and not run if not Debian based platform since this is only compatible with Debian.

i fixed this in a local copy by adding:

case node['platform_family']
when "debian"

at the begining of default.rb recipe and then:

end

at the end of the file so it only runs when on debian systems removing the compatibility issue with windows.

@chr4
Copy link
Member

chr4 commented Dec 22, 2016

Thanks for the pull-request! I'm wondering whether there is a cleaner solution for skipping unsupported systems. Would defining supported distributions in metadata.rb help? I'm not sure if that's evaluated.

Another solution would be to only include the recipe on Linux systems in the first place (in your wrapper cookbook). I think this is a common problem basically for every single cookbook that only supports Linux but not Windows (and vice versa) but I've never seen a clause like this in the cookbook's recipes.

@axelrtgs
Copy link
Author

axelrtgs commented Dec 22, 2016

i do not think the metadata.rb is evaluated as its already defined and doesn't do anything.

I know the runit cookbook by chef does a similar evaluation mostly to see if its RHEL or DEBIAN but this also means it wont run on windows. and the apt cookbook has a only_if apt_installed so since apt is not installed on windows or non compatible platforms it gets skipped and windows is also happy.

EDIT: as for the wrapper solution, I'm setting the attributes and calling the recipe from a role so i do not have a wrapper cookbook so that wouldn't work for me. and using the case statement you add support for not using wrappers.

Copy link

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

I verified that supports will not result in chef either avoiding the recipe or failing to run altogether. This is quite old and could remove the bits for ubuntu 12.04 as it's no longer supported.

@chr4
Copy link
Member

chr4 commented May 11, 2018

I'd be fine with including this as soon as the workaround for ubuntu 10.04 is removed, as it won't hurt anybody.

I'm not sure though whether such exceptions should rather be placed in a wrapper cookbook.

@axelrtgs
Copy link
Author

The question about being placed in a wrapper cookbook is that because its not a custom resource i would say most people set the attributes in the environment and call the cookbook directly without a wrapper.

Requiring people to absolutely use a wrapper to support windows and linux or forcing them to create separate platform dependent roles i think would be a worse solution.

most of the community cookbooks supporting only one platform (and i think all of them written by chef) have the platform check and will not run the recipes otherwise issues occur with cross platform compatibility. I can give you some examples if you like.

For the workaround i only kept it because it was in the original code when i created the PR, i will be happy to merge in master and remove that workaround as part of my PR to kill two birds with one stone.

P.S. I totally forgot about this open PR. thanks for following up

@chr4
Copy link
Member

chr4 commented May 11, 2018

Ok, I'd apprechiate an update and rebase!

I'm willing to take it in, as I can't see any downside, I never ran into this problem, because I always found configuring Chef with attributes was almost unusable as soon as it got beyond the very basics. But that might be just me :)

@axelrtgs
Copy link
Author

So i just took a look at the latest changes in the repo for the chef13 compatibility and i think i know how to retain backwards compatibility with using the default recipe and not putting the defaults on the resource.

Also, if it would be OK with you I would like to try and convert this into a truly custom resource and clean up some other legacy things.

I will close this PR since without thinking I delete my fork I was working off of and I will submit a new PR with all the changes. Hopefully that will bring it into a Chef13 compliant state while retaining as much backwards compatibility as possible and allowing a supermarket release.

I will try to get to it next week. :)

@chr4
Copy link
Member

chr4 commented May 11, 2018

Thanks in advance for all your effords, looking forward for the new pull request!

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.

3 participants