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

Added option to retry the block given #428

Merged
merged 5 commits into from Mar 13, 2019

Conversation

Projects
None yet
5 participants
@teclator
Copy link
Contributor

commented Mar 10, 2019

Problem

When an exception or error occurs in the qa SCC proxy, the shown description is not user friendly, it shows all the text in the dialog and only permits to accept it.

Solution

  • Added an option to connection_helper for retrying the given blockn when some exception occurs.
  • Added a details button to show the details of the occurred error in the retry dialog.

Testing

  • Added uni test
  • Tested manually

Screenshots

Screenshot_testing_2019-03-13_09:06:14

Screenshot_testing_2019-03-11_00:08:39

@coveralls

This comment has been minimized.

Copy link

commented Mar 10, 2019

Coverage Status

Coverage decreased (-0.02%) to 72.176% when pulling ba1d1e0 on retry_block into 9e5f74d on master.

@teclator teclator force-pushed the retry_block branch from 6ff47dc to eae4f2a Mar 10, 2019

@teclator teclator requested a review from lslezak Mar 11, 2019

Show resolved Hide resolved src/lib/registration/connect_helpers.rb Outdated
Show resolved Hide resolved src/lib/registration/connect_helpers.rb Outdated
@@ -142,7 +148,12 @@ def self.catch_registration_errors(message_prefix: "",
log.error "JSON parse error"
# update the message when an old SMT server is found
check_smt_api(e.message)
report_error(message_prefix + _("Cannot parse the data from server."), e.message)
msg = message_prefix + _("Cannot parse the data from server.")

This comment has been minimized.

Copy link
@lslezak

lslezak Mar 11, 2019

Member

I'm not sure if we should retry on JSON::ParserError, the server might register the system properly (i.e. eating one of the customer subscription, but only the response is malformed or corrupted when transmitting over network).

But as this was mainly an issue with the SCCProxy then let the user decide, this should not happen with normal SCC...

This comment has been minimized.

Copy link
@teclator

teclator Mar 11, 2019

Author Contributor

I'm not sure if we should retry on JSON::ParserError, the server might register the system properly (i.e. eating one of the customer subscription, but only the response is malformed or corrupted when transmitting over network).

MMM in that case, when retrying the registration, it will be skipped as detected as already register, isn't it?

But as this was mainly an issue with the SCCProxy then let the user decide, this should not happen with normal SCC...

Yes, this looks more like the old registration server error than a normal SCC one, although the notes in Trello suggested to add the [Retry] option

This comment has been minimized.

Copy link
@lslezak

lslezak Mar 11, 2019

Member

I'm fine with adding retry here, my point is that the user should know the consequences... But that's actually the case also for the timeout exception. It might happen that the server registers the client correctly but because of some network issue the client gets timeout...

This comment has been minimized.

Copy link
@teclator

teclator Mar 12, 2019

Author Contributor

Exactly, and the timeout could happen at any time, that was what happened with the SCC qa proxy but there also the error is handled incorrectly.

This comment has been minimized.

Copy link
@teclator

teclator Mar 12, 2019

Author Contributor

I removed the retry, but could add it back if wanted.

@teclator teclator force-pushed the retry_block branch 3 times, most recently from 52cca8c to 3085e9a Mar 12, 2019

Show resolved Hide resolved src/lib/registration/connect_helpers.rb Outdated
Show resolved Hide resolved src/lib/registration/connect_helpers.rb Outdated
Show resolved Hide resolved src/lib/registration/registration_ui.rb Outdated

@teclator teclator force-pushed the retry_block branch from 3ae0547 to 986dd74 Mar 12, 2019

@lslezak
Copy link
Member

left a comment

LGTM

@@ -142,6 +142,7 @@ def self.catch_registration_errors(message_prefix: "",
# update the message when an old SMT server is found
check_smt_api(e.message)
details_error(message_prefix + _("Cannot parse the data from server."), e.message)

This comment has been minimized.

Copy link
@teclator

teclator Mar 13, 2019

Author Contributor

while Yast::Report returned nil, details_error reports :ok so, added the return value explicitly as we have in the other rescued exceptions

@@ -55,7 +55,7 @@ def initialize(registration)
def register_system_and_base_product
product_service = nil
# TRANSLATORS: Popup error message prefix
error_options = { message_prefix: _("Registration failed.") }
error_options = { message_prefix: _("Registration failed.") + "\n\n" }

This comment has been minimized.

Copy link
@teclator

teclator Mar 13, 2019

Author Contributor

Added new lines to the message prefix:

Screenshot_testing_2019-03-13_09:06:14

@lslezak
Copy link
Member

left a comment

LGTM

@teclator teclator merged commit 248ef2c into master Mar 13, 2019

4 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 72.176%
Details

@teclator teclator deleted the retry_block branch Mar 13, 2019

@yast-bot

This comment has been minimized.

Copy link

commented Mar 13, 2019

✔️ Internal Jenkins job #27 successfully finished
✔️ Created IBS submit request #186947

@yast-bot

This comment has been minimized.

Copy link

commented Mar 13, 2019

✔️ Public Jenkins job #32 successfully finished
✔️ Created OBS submit request #684635

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.