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
fixes #10788 - add foreman-admin for admin tasks #2452
Conversation
There were the following issues with the commit message:
Guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
renamed commit message |
fixed rubocop warnings |
#!/usr/bin/env ruby | ||
# -*- encoding: utf-8 -*- | ||
|
||
require 'rubygems' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fine without this line in Ruby 1.9, not sure about 1.8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is required for 1.8
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Foreman core doesn't support 1.8, it can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't ruby's version on el6
outside of the SCL 1.8
though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but see below, if packaged in Foreman then we can simply depend on SCL and use 1.9+ on all platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
Needs fleshing out some more, removal of all of the TODOs. |
@komidore64 Can you fix rubocop errors please? |
TODO still needs doing, and how do you plan for this to be extended from plugins? It should also have a man page. |
@domcleal haha, well yeah. they're TODOs because i'm still working on it :)
As for plugins adding more commands, what they'd need to do is add a file to Take
example: module ForemanAdmin
class CountCommand < ForemanAdmin::ExternalCommand
command_name 'countdown' # required
description 'prints out numbers counting downward' # required
# regular clamp-style parameters and options
option ['--prefix', '-p'], 'PREFIX', 'String to prefix each count', :default => ''
parameter 'COUNT', 'Number to start from' do |c|
Integer(c)
end
# override is required otherwise command will simply exit
def execute
count.downto(1) { |c| puts "#{prefix}#{c}" }
end
end
end
example: class DebugCommand < ForemanAdmin::ExternalCommand
command_name 'generate-debug' # required
description 'Create a foreman-debug tarball for debugging purposes' # required
external_invocation '/usr/sbin/foreman-debug' # required
parameter "[ARGS] ...", "args", :hidden => true
# optional override (this is a clamp method)
def help
`#{external_invocation} -h`.gsub(external_invocation, File.expand_path(__FILE__))
end
# optional override (this is a ForemanAdmin::ExternalCommand method)
def external_command
"#{external_invocation} #{args_list}"
end
end |
Most of what I just explained above will make it into foreman-admin's man page in one form or another. |
There were the following issues with the commit message:
Guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
argh @theforeman-bot! @ShimShtein addressed rubocop messages. |
subcommand(DebugCommand.command_name, DebugCommand.description, DebugCommand) | ||
|
||
preload_constants = ForemanAdmin.constants | ||
Dir.glob('/etc/foreman-admin.d/*.rb').each do |file| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be relative to Foreman's root, and definitely not in /etc for code.
|
||
class DebugCommand < ForemanAdmin::ExternalCommand | ||
command_name 'generate-debug' | ||
description 'Create a foreman-debug tarball for debugging purposes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lacking i18n support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foreman-debug is only in English?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but Foreman is generally i18n capable, as is our existing main CLI, so I'd expect a new CLI to be i18n capable too. Especially if you're trying to make it more user-centric than the existing mix of tools.
Seems that with stuff like extensions and locales that using the existing Hammer framework would be quite useful. Any reason this couldn't just be developed in its existing plugins in some way? |
@domcleal hammer was considered, but as you stated earlier hammer is not part of Foreman core. Since these actions take place on the filesystem (not inside of the rails environment), @mccun934 and I are wanting these actions to only be available to the system's root user which can best be achieved by operating outside of the Foreman/Rails environment entirely instead of using hammer with an API. |
Right, but at least we have an existing plugin distribution method for it, existing locale support etc. Reinventing that properly is work.
It doesn't need to operate over the API though, that's only what hammer_cli_foreman usually does, not hammer_cli. |
@komidore64, |
Portions of foreman-admin need more development, so I'm going to close this. |
moving into foreman proper due to conversation in theforeman/foreman-packaging#672
still to do:
clamp
to foreman deps