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 #26355 - background template rendering #414

Merged
merged 4 commits into from Apr 15, 2019

Conversation

Projects
None yet
5 participants
@ezr-ondrej
Copy link
Member

commented Mar 14, 2019

I have added scheduling, so you can schedule report and download it once it is ready through second command.

The schedule command has --wait option, which will wait for the result actively.

@ofedoren
Copy link
Member

left a comment

Thanks, @ezr-ondrej! I know it's WIP, but I'd rather review it ASAP, so you have something to begin with :)

See inline comments, otherwise everything looks fine. Also, there is need to upgrade our apidoc data (test/data/1.21.1 or 1.22) for newer version of Foreman so your tests can pass (I've tried it locally and they seemed to be passing).

Show resolved Hide resolved lib/hammer_cli_foreman/report_template.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/report_template.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/report_template.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/report_template.rb Outdated
Show resolved Hide resolved test/functional/report_template_test.rb Outdated

@ezr-ondrej ezr-ondrej changed the title [WIP] Fixes #26355 - background template rendering Fixes #26355 - background template rendering Mar 30, 2019

@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:report_templates_background branch from c1036e1 to 8c525ed Mar 30, 2019

Show resolved Hide resolved test/test_helper.rb Outdated
@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

Thanks for the pushes in right direction @ofedoren I believe it is ready, as of foreman PR were merged.
Just do not know what to do with the data, because the API is going to get to foreman 1.22, but it is not done, so I should not put the data now, right?

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

@ezr-ondrej I think you can do 1.22 from current foreman develop. If there is anything else that will slip in last minute in 1.22 and it is needed for testing in hammer we can always update.

@ofedoren
Copy link
Member

left a comment

I agree with @mbacovsky and I'm sure you know how to do it, but if not here's readme: https://github.com/theforeman/hammer-cli-foreman/tree/master/test/data (if I'm not mistaken we generate data without Katello plugin, but pure Foreman).

Also, see the last notes inline :)

if option_wait?
poll_for_report(data)
else
puts data['job_id']

This comment has been minimized.

Copy link
@ofedoren

ofedoren Apr 5, 2019

Member

Printing job id only is not very user-friendly approach to tell user what's happening now, isn't it? I mean, what do you think about doing something like print_message(_('The report has been scheduled. Job ID: %{job_id}') % { job_id: data['job_id'] })?

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Apr 9, 2019

Author Member

Yeah, sounds much more useful 👍 thanks.

response = send_request
if response.code == 204
print_message(_('The report is not ready yet.'))
return HammerCLI::EX_OK

This comment has been minimized.

Copy link
@ofedoren

ofedoren Apr 5, 2019

Member

nitpick: you can lower this statement to the end of the method and remove return.

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Apr 9, 2019

Author Member

🎯 👍

else
puts response.body
end
return HammerCLI::EX_OK

This comment has been minimized.

Copy link
@ofedoren

ofedoren Apr 5, 2019

Member

nitpick: redundant return.

def handle_success(response)
if option_path
filepath = store_response(response)
print_message(_('The response has been saved to %{path}s.'), {:path => filepath})

This comment has been minimized.

Copy link
@ofedoren

ofedoren Apr 5, 2019

Member

Redundant s at the end of the sentence.

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Apr 9, 2019

Author Member

🎯 👍

@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Blocked by #418 or at least just #409 as without it, tests won't be green on new data.

@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:report_templates_background branch from adb6039 to 7dd5547 Apr 9, 2019

@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

Rebased on top of #409 so is no longer blocked, #418 is not blocking this.

@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:report_templates_background branch from 7dd5547 to 746b08d Apr 11, 2019

def execute
response = send_request
if response.code == 204
print_message(_('The report is not ready yet.'))

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Apr 12, 2019

Author Member

Is there some Exit code to suggest that?

This comment has been minimized.

Copy link
@ofedoren

ofedoren Apr 12, 2019

Member

Well, I could suggest HammerCLI::EX_RETRY but I'm sure it will flood the terminal with that message in some cases, which is not quite good. HammerCLI::EX_OK is good enough for it. If you meant something else, ping me on irc (ofedoren) so we can merge it today.

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Apr 12, 2019

Member

EX_RETRY will lead in re-executing the command again which in the end will make hammer loop here till the report is delivered. If this is not what we want I'd suggest to look at https://github.com/theforeman/hammer-cli/blob/master/lib/hammer_cli/exit_codes.rb
EX_TEMPFAIL may be a good candidate. From certain perspective (expected result, no error) the EX_OK is fine too if no better option is found.

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Apr 12, 2019

Author Member

Ok, I have rolled out with EX_TEMPFAIL 👍 Thank you both for advises!

@ofedoren
Copy link
Member

left a comment

For me it's ready to merge if you're ready :)

@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@ofedoren I believe it is ready :)

@ares

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I'm also ready :-) could this be merged please @ofedoren :-)

@ofedoren

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@ares. sure :)

@ezr-ondrej. merging then, thanks! :)

@ofedoren ofedoren merged commit d36721b into theforeman:master Apr 15, 2019

1 check passed

hammer Build finished. 5280 tests run, 0 skipped, 0 failed.
Details
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.