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

Add rake task to import custom buttons #17726

Merged
merged 2 commits into from
Aug 2, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jul 18, 2018

We need to be able to import custom buttons via a rake task.

Related export PR: #17699

Importing

Import cbs:
rake evm:import:custom_buttons -- --source /Users/duhlmann/manageiq/custom_buttons

Note: this works either for a single file or for a directory.

The command for a single file looks like this:
rake evm:import:custom_buttons -- --source /Users/duhlmann/manageiq/custom_buttons.yaml

@miq-bot miq-bot added the wip label Jul 18, 2018
@d-m-u d-m-u force-pushed the custom_button_import branch 4 times, most recently from 6444ffe to 4f59749 Compare July 18, 2018 18:39
@d-m-u
Copy link
Contributor Author

d-m-u commented Jul 18, 2018

@miq-bot add_label enhancement, wip

@d-m-u d-m-u changed the title [WIP] Start of rake task to import custom buttons [WIP] Add rake task to import custom buttons Jul 18, 2018
@d-m-u d-m-u changed the title [WIP] Add rake task to import custom buttons [WIP] Add rake task to export custom buttons Jul 18, 2018
@d-m-u d-m-u changed the title [WIP] Add rake task to export custom buttons [WIP] Add rake task to import custom buttons Jul 18, 2018
@gmcculloug gmcculloug self-assigned this Jul 19, 2018
@d-m-u d-m-u changed the title [WIP] Add rake task to import custom buttons Add rake task to import custom buttons Jul 19, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jul 19, 2018

@tinaafitz @syncrou @eclarizio
@whoever_wants_to_review_me

@miq-bot miq-bot removed the wip label Jul 19, 2018
@d-m-u d-m-u changed the title Add rake task to import custom buttons [WIP] Add rake task to import custom buttons Jul 20, 2018
@miq-bot miq-bot added the wip label Jul 20, 2018
@d-m-u d-m-u force-pushed the custom_button_import branch 2 times, most recently from ada603c to c1abda5 Compare July 20, 2018 15:01
@d-m-u d-m-u changed the title [WIP] Add rake task to import custom buttons Add rake task to import custom buttons Jul 20, 2018
@miq-bot miq-bot removed the wip label Jul 20, 2018
@d-m-u d-m-u changed the title Add rake task to import custom buttons [WIP] Add rake task to import custom buttons Jul 20, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jul 20, 2018

Okay, fine. WIP cause 1) tests failing 2) yaml linting 3) need flag to overwrite existing thingies being imported. May handle 3 in subsequent PR.

@miq-bot miq-bot added the wip label Jul 20, 2018
@d-m-u d-m-u force-pushed the custom_button_import branch 2 times, most recently from 1ebe6ff to 1857a9a Compare July 20, 2018 16:55
@syncrou
Copy link
Contributor

syncrou commented Jul 20, 2018

@d-m-u - I found I needed the file extension e.g. rake evm:import:custom_buttons -- --source /Users/duhlmann/manageiq/custom_buttons.yaml to get the task to load the file.

@d-m-u d-m-u force-pushed the custom_button_import branch 3 times, most recently from dba2f15 to 74a43be Compare July 23, 2018 12:15
@d-m-u d-m-u force-pushed the custom_button_import branch 3 times, most recently from 2d7fff4 to 77ecb17 Compare July 27, 2018 12:31
Dir.glob(glob) do |filename|
begin
import_custom_buttons(filename)
rescue
Copy link
Member

Choose a reason for hiding this comment

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

Suggest: rescue StandardError

end

if obj['userid'].present?
check_user(obj)
Copy link
Member

Choose a reason for hiding this comment

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

Should check_user be passed new_obj?

Since code complexity is high on this method maybe we should break the if-blocks out into separate methods.

klass.create!(obj['attributes'].except('guid')).tap do |new_obj|
  add_children(obj, new_obj)
  add_associations(obj, new_obj)
  check_user(new_obj)
end

@d-m-u d-m-u force-pushed the custom_button_import branch 2 times, most recently from b1018d1 to 61658f7 Compare July 27, 2018 16:22
@gmcculloug gmcculloug requested a review from eclarizio July 28, 2018 14:51
klass.create!(obj['attributes'].except('guid')).tap do |new_obj|
add_children(obj, new_obj)
add_associations(obj, new_obj)
check_user(new_obj) if new_obj.kind_of?(CustomButton)
Copy link
Member

Choose a reason for hiding this comment

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

This logic should be called from custom_button_post. Then you do not need this check for the class type.

@d-m-u d-m-u force-pushed the custom_button_import branch from 61658f7 to fbc0ecc Compare July 31, 2018 12:45
@gmcculloug
Copy link
Member

@eclarizio PTAL

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

Looks good aside from a question I have with the error handling. 👍

begin
import_custom_buttons(filename)
rescue StandardError
warn("Error importing #{options[:source]} due to #{$ERROR_INFO} at #{$ERROR_POSITION}")
Copy link
Member

Choose a reason for hiding this comment

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

Two things here (I think?), first is that, if we get here, aren't we getting here because of the raise from here: https://github.com/ManageIQ/manageiq/pull/17726/files#diff-a0ce4acbb7481b72b585132391c2dd4cR28, which means that this will just say something like "error because of raise at line 28?" and not give us actual good info?

Second is that I think there is the potential for YAML.load_file to fail if the YAML is messed up, in which case I don't remember if StandardError will catch it, cause it's a Psych error or something I think. Maybe?

In either case, maybe some specs ensuring the errors are doing what you want them to do are a good solution to my worries 😄

@d-m-u d-m-u force-pushed the custom_button_import branch 3 times, most recently from a215315 to 1f378b8 Compare August 1, 2018 16:01
@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2018

This pull request is not mergeable. Please rebase and repush.

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 1, 2018

I think these failures are unrelated.

@d-m-u d-m-u force-pushed the custom_button_import branch 2 times, most recently from 8692d8c to 4d4510d Compare August 1, 2018 19:27
@d-m-u d-m-u force-pushed the custom_button_import branch from 4d4510d to e2d1697 Compare August 1, 2018 20:24
@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2018

Checked commits d-m-u/manageiq@f8ca9bf~...e2d1697 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

lib/task_helpers/imports/custom_buttons.rb

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

👍

@gmcculloug gmcculloug merged commit 4f8556c into ManageIQ:master Aug 2, 2018
@gmcculloug gmcculloug added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 2, 2018
@d-m-u d-m-u deleted the custom_button_import branch February 1, 2019 20:47
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