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

Fixes #24489 - add a command for report templates #396

Merged
merged 3 commits into from Nov 6, 2018

Conversation

Projects
None yet
4 participants
@mbacovsky
Copy link
Member

commented Nov 2, 2018

This PR replaces the original report templates PR. Features included:

  • original commit with basic commands
  • updated API docs with the report templates endpoints
  • clone command
  • interactive mode for update and create which opens the template in a $EDITOR to do changes
  • --inputs in the generate command
  • inputs listed in the info command

TODO:

  • tests
@mbacovsky

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2018

There is a new helper in hammer core needed for testing the interactive editing - theforeman/hammer-cli#294.
Also to test the clone command there is a patch needed in Foreman core theforeman/foreman#6197.

@ares

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

rendering works great with inputs too, also template creation from files works nicely, ack from user testing perspective 👍

@tstrachota
Copy link
Member

left a comment

Thanks @mbacovsky! Editing the templates in vi is a nice feature.
I added some comments. I'm going to test it now.

HammerCLIForeman::References.timestamps(self)
HammerCLIForeman::References.taxonomies(self)
collection :inputs, _("Template inputs") do
field :name, _('Name')

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Does it make sense to show input's id?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Nov 6, 2018

Author Member

Sort of, just if you want to delete the inputs using template-input delete command. It is minor case, so id doesn't have to be there if it improve UX

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

I'd be for adding IDs as we do it everywhere else - with type Id. This way it won't be shown unless users pass --show-ids.

def print_data(response)
if option_path
if response.body.empty?
print_message "Response success but empty body was returned - file was not saved"

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Mark for translation please.
When can this situation happen btw?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Nov 6, 2018

Author Member

Good point, maybe @ares would know? Otherwise it can be removed.

This comment has been minimized.

Copy link
@ares

ares Nov 6, 2018

Member

I saw this when I used --path but the result of report was blank. E.g. I specified host search term not resulting in any host.

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Nov 6, 2018

Author Member

I see now. I don't think we need to handle this case. Empty report is IMO okay to store or print. That it is not result of an error we indicate by zero exit code.


def store_response(response)
# get file name from header, remove leading and trailing quotes
filename = response.headers[:content_disposition].match(/filename=(.*)/)[1].chop.reverse.chop.reverse

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

If the chop & reverse harakiri is just to remove the leading and the last character, there are more readable ways:

  • str[1..-2]
  • str.gsub(/^'|'$/, '')
filename = response.headers[:content_disposition].match(/filename=(.*)/)[1].chop.reverse.chop.reverse
path = option_path.dup
path << '/' unless path.end_with? '/'
raise "Cannot save file: #{path} does not exist" unless File.directory?(path)

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

I guess this should be translated.

end

def success_message
super || _('The response has been saved to %s.') % @filepath

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

I'd prefer overriding the run method instead of saving file path into instance variable and changing behavior of print_data. That would imho be more readable.

raise "Cannot save file: #{path} does not exist" unless File.directory?(path)
@filepath = path + filename
File.write(@filepath, response.body)
print_success_message(response)

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Please use print_success_message(response, :file_path => filepath) instead of passing the value via instance variable.


def request_params
params = super
params['input_values'] = option_inputs || {}

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Maybe use default value in the option definition?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Nov 6, 2018

Author Member

I'd prefer not to to distinguish inputs were not provided by the user

end


class UpdateCommand < HammerCLIForeman::UpdateCommand

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Shouldn't one of --interactive and --file be required here too?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Nov 6, 2018

Author Member

I don't think so, you can update also other attributes (name, default)

@@ -519,4 +519,48 @@ def self.success_message(msg = nil)
super(msg) || default_message(_('The %{resource_name} has been disassociated.'))
end
end

class DownloadCommand < HammerCLIForeman::SingleResourceCommand

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Do you plan to re-use this command somewhere else?

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Nov 6, 2018

Author Member

I'm not sure what other API endpoints use this patterns. @ares may know.

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Well I see some potential in using it for partition tables and provisioning templates commands. I just wanted to know whether there are any plans for such future refactoring.


def store_response(response)
# get file name from header, remove leading and trailing quotes
filename = response.headers[:content_disposition].match(/filename=(.*)/)[1].chop.reverse.chop.reverse

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

What if the header is nil? Should we catch this?


def store_response(response)
if response.headers.key?(:content_disposition)
suggested_filename = response.headers[:content_disposition].match(/filename="(.*)"/)

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

👍 Elegant solution for removing the quotes ;)

response = send_request
if option_path
filepath = store_response(response)
print_message(_('The response has been saved to %s.') % filepath)

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Nitpick: I'm not going to block the PR for this, but note that you can use print_message(_('The response has been saved to %{path}s.'), {:path => filepath}). The benefit is that structured output adapters will print both the message and file path under separate keys -> no need to parse it if you want to use it for scripting.

if response.headers.key?(:content_disposition)
suggested_filename = response.headers[:content_disposition].match(/filename="(.*)"/)
end
filename = suggested_filename ? suggested_filename[1] : default_filename

This comment has been minimized.

Copy link
@tstrachota

tstrachota Nov 6, 2018

Member

Nice, thanks for adding the default name.

@mbacovsky mbacovsky force-pushed the mbacovsky:24489_report_templates branch 2 times, most recently from 06c5866 to ca38fe8 Nov 6, 2018

@mbacovsky mbacovsky force-pushed the mbacovsky:24489_report_templates branch from ca38fe8 to b444800 Nov 6, 2018

@tstrachota

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Thanks @mbacovsky and @ares, good job!

@tstrachota tstrachota merged commit ebc7bd6 into theforeman:master Nov 6, 2018

1 check passed

hammer Build finished. 4053 tests run, 0 skipped, 0 failed.
Details
@tstrachota

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

BTW this is a nice candidate for the next community demo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.