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 a button for re-checking authentication status for providers #8912

Merged
merged 9 commits into from Jun 6, 2016

Conversation

AparnaKarve
Copy link
Contributor

@AparnaKarve AparnaKarve commented May 23, 2016

Addresses a part of #8643.

While the other major UI changes mentioned in #8643 for the ems editors are still underway, we can move forward with this particular change which basically adds a Re-check Authentication button in the EMS Summary screen to update the Authentication status.

screen shot 2016-06-01 at 7 06 53 am

@AparnaKarve
Copy link
Contributor Author

@Fryguy @jrafanie Please review the new method authentication_check_queue in authentication_mixin 7486a14. It's pretty much based on one of the existing _queue methods in that mixin.

@AparnaKarve
Copy link
Contributor Author

/cc @Ladas @juliancheal

@AparnaKarve
Copy link
Contributor Author

@miq-bot add_label wip,ui,providers

@@ -273,6 +273,27 @@ def authentication_check(*args)
return status == :valid, details
end

def authentication_check_queue(*args)
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the nearly identical authentication_check_types_queue?

It's what we call from the schedule worker on a schedule for each model that includes the authentication mixin.

zone = MiqServer.my_server.zone
assoc = name.tableize
assocs = zone.respond_to?(assoc) ? zone.send(assoc) : []
assocs.each(&:authentication_check_types_queue)

Copy link
Member

Choose a reason for hiding this comment

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

If there's something you can't do, please provide a test or script demonstrating what's missing and I'll try to fix the code to do what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrafanie authentication_check_types_queue works just as great! Thanks for pointing me to this method.
However, the only thing that needs to happen before we can use this method is to make sure that ems types that have authentication types other than default should have this method available - authentications_to_validate.

Copy link
Member

Choose a reason for hiding this comment

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

Good point @AparnaKarve.

The concept was that classes could dictate what authentication types it used to authenticate via authentications_to_validate.

Another way is to pass the types, see here. If you don't specify the types, then use authentications_to_validate will be used.

@AparnaKarve AparnaKarve force-pushed the reverify_auth_status branch 2 times, most recently from 2527a5b to 9482367 Compare May 25, 2016 20:19
@miq-bot
Copy link
Member

miq-bot commented May 25, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@AparnaKarve AparnaKarve force-pushed the reverify_auth_status branch 3 times, most recently from 4ca4f75 to 8934bc5 Compare June 1, 2016 02:21
@AparnaKarve
Copy link
Contributor Author

@Ladas Can you review this?
I've added the 'Re-check Authentication' button for ems_cloud, ems_infra and ems_container.
As discussed, also updated the alert message - dd94f8b

@Ladas
Copy link
Contributor

Ladas commented Jun 2, 2016

@AparnaKarve hm one question, does this recheck authentication of all attached endpoints? E.g. for OpenStack you can have default and events endpoint there, while only amqp can be rechecked

If ^ is working, it looks good to me, though I don't have a space to actually try this now.

@miq-bot
Copy link
Member

miq-bot commented Jun 2, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@AparnaKarve
Copy link
Contributor Author

@Ladas It rechecks authentications of all attached endpoints.
Basically, whatever authentications you see on the Summary page, those will be rechecked.

For e.g - There could be a situation where an OpenStack provider has both default and amqp endpoints defined, so for that provider, both authentications will be rechecked.
Likewise, there could be an OpenStack provider with only the default endpoint defined, in which case only the default authentication will be rechecked.

So just to clarify the above, you cannot "cherry-pick" (for the lack of a better word) what authentication type to recheck.

Does that sound OK to you?

@Ladas
Copy link
Contributor

Ladas commented Jun 2, 2016

@AparnaKarve yeah, that sounds good. In that case looks 👍 to me :-)

@miq-bot
Copy link
Member

miq-bot commented Jun 2, 2016

Checked commits AparnaKarve/manageiq@08bb4cb~...d3d58a4 with ruby 2.2.4, rubocop 0.37.2, and haml-lint 0.16.1
11 files checked, 8 offenses detected

app/controllers/ems_common.rb

app/helpers/application_helper/toolbar/ems_cloud_center.rb

  • 🔶 - Line 75, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • 🔶 - Line 89, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/ems_container_center.rb

  • 🔶 - Line 81, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • 🔶 - Line 95, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar/ems_infra_center.rb

  • 🔶 - Line 86, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • 🔶 - Line 100, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

@AparnaKarve
Copy link
Contributor Author

AparnaKarve commented Jun 2, 2016

@Ladas Thanks for the confirmation above.
Now if only I could get a volunteer to test this PR :) -- I would think we do need this for darga.
@Ladas, tbh, you are the perfect candidate to test this... just saying 😉

@gtanzillo gtanzillo closed this Jun 2, 2016
@gtanzillo gtanzillo reopened this Jun 2, 2016
@AparnaKarve
Copy link
Contributor Author

@miq-bot remove_label wip

@AparnaKarve AparnaKarve changed the title [WIP] Add a button for re-checking authentication status for providers Add a button for re-checking authentication status for providers Jun 2, 2016
@miq-bot miq-bot removed the wip label Jun 3, 2016
@chessbyte chessbyte assigned h-kataria and unassigned AparnaKarve Jun 3, 2016
def skip?
!@record.is_available?(:authentication_status)
end
end

Choose a reason for hiding this comment

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

You don't need to create several classes with the same code.

If the behavior you need here just checks for auth status on the record, you can just create 1 class for that and share the class for all the buttons

@Fryguy Fryguy merged commit 027cf91 into ManageIQ:master Jun 6, 2016
@Fryguy Fryguy added this to the Sprint 42 Ending June 20, 2016 milestone Jun 6, 2016
chessbyte pushed a commit that referenced this pull request Jun 7, 2016
Add a button for re-checking authentication status for providers
(cherry picked from commit 027cf91)
@abonas
Copy link
Member

abonas commented Jul 13, 2016

@AparnaKarve middleware provider seems out of scope for this feature and PR - any reason why?

@AparnaKarve
Copy link
Contributor Author

@abonas The main reason would be this feature was targeted for Darga.
I can create a separate PR for middleware for this feature.

@abonas
Copy link
Member

abonas commented Jul 13, 2016

@abonas The main reason would be this feature was targeted for Darga.
I can create a separate PR for middleware for this feature.

thanks @AparnaKarve , it can definitely be a separate PR.

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.

None yet

10 participants