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

Remove chef related code #212

Closed
wants to merge 1 commit into from
Closed

Conversation

ares
Copy link
Member

@ares ares commented Sep 13, 2014

Code was extracted to separate chef plugin. See
https://github.com/theforeman/smart_proxy_chef

@dmitri-d
Copy link
Member

I'm ok with this, @domcleal?

@dmitri-d
Copy link
Member

@ares: do you need a review of the extracted plugin, or is it a verbatim copy?

@domcleal
Copy link
Contributor

I'm fine in principle with the move.

@ares
Copy link
Member Author

ares commented Sep 15, 2014

@witlessbird it's a copy with additional featues, if you're interested you can go over https://github.com/theforeman/smart_proxy_chef and add comments there

@ares
Copy link
Member Author

ares commented Sep 15, 2014

We should put some warning to release notes, users have to install the plugin if they relied on this functionality. The plugin is not yet packed though.

@domcleal
Copy link
Contributor

Indeed, but should also be handled through packaging (obsoletes).

@dmitri-d
Copy link
Member

@ares: I left you a comment in the closed PR at https://github.com/theforeman/smart_proxy_chef. It mostly looks good to me, the only issue is that you are using an 'api' namespace in this module, which I think sounds too generic and can potentially cause conflicts (or modules not being loaded) down the line. I assume this namespace was used by chef module before the whole modularization?

@ares
Copy link
Member Author

ares commented Sep 16, 2014

@witlessbird thanks, the /api was there before modularization (and I believe is still there until we merge this PR) and the path is there so we have same URL if we point chef-clients to foreman directly or smart-proxy (which optionally does the authentication). I will move it later, probably when I write-up some new installation docs or work on installer support for this.

@domcleal
Copy link
Contributor

@ares could you rebase, assign a redmine ticket and let's get this into 1.7 please

@ares
Copy link
Member Author

ares commented Oct 20, 2014

I'll try to build package for the plugin first since we would drop packaging for this functionality

@ares
Copy link
Member Author

ares commented Oct 29, 2014

I didn't make it. @domcleal I know it's past 1.7 branching but this does not add feature (it removes it) so maybe we could still get it into 1.7.

@rvrignaud
Copy link
Contributor

I tested this PR with smart-proxy and smart_proxy_chef installed. We have same functional level. I would be I favor of merging this PR to avoid unnecessary complexity.

@domcleal
Copy link
Contributor

@ares I'm happy to have this in 1.7, but only on the basis that the packages are sorted out ASAP after merging. (Before RC2 - if longer, then I'll probably revert in 1.7-stable as we need to ship something functioning.)

@@ -1,3 +0,0 @@
group :chef do
gem 'chef', ">= 11.6.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

mixlib-shellout is also in test.rb because of chef, it should be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

mixlib-shellout removed

@domcleal
Copy link
Contributor

domcleal commented Nov 6, 2014

Thanks @ares, merged as ed82c92.

@domcleal domcleal closed this Nov 6, 2014
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.

5 participants