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

Fix missing request type in OnSystemRequest #315

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

AKalinich-Luxoft
Copy link
Contributor

@AKalinich-Luxoft AKalinich-Luxoft commented Apr 14, 2020

Fixes #313 and #339

This PR is ready for review.

Testing Plan

Will be tested manually

Summary

There was found a few issues while working with HMI in ExternalPolicies = true mode:

  1. HMI is sending OnSystemRequest twice on start
  2. First OnSystremRequest notification does not contain requestType parameter (looks like a regression caused by Fix/Add request type param to pack and unpack requests #293)
  3. HMI does not send OnSystemRequest after the retry attempts exceeded

To fix that, HMI logic was updated to launch retry sequence only after the response received by pack client. By some reason, retry sequence was additionally been started right after GetPolicyConfigurationData response. This extra call has been removed to make HMI send notification just once. Also, retry sequence function was updated to use request params received by pack client (if available).

CLA

There was found a few issues while working with HMI
in ExternalPolicies = true mode:
1. HMI is sending OnSystemRequest twice
2. First OnSystremRequest notification does not contain
requestType parameter

To fix that, HMI logic was updated to launch retry sequence
only after the response received by pack client. By some
reason, retry sequence was additionally been started right
after GetPolicyConfigurationData response. This extra call
has been removed to make HMI send notification just once.
Also, retry sequence function was updated to use request
params received by pack client (if available).
*/
policyUpdateRetry: function(abort) {
policyUpdateRetry: function(action, request_params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@AKalinich-Luxoft just a suggestion. How about removing action parameter? The only check for this parameter is action !== 'ABORT'. So, maybe, it would be better to have an additional abortion function? Moreover, as far as I understand, if isIterationInProgress == true, abort parameter is pointless. And when we get to abort code block, the timer is already cleaned up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkorniichuk good idea. I made some code refactoring in 9567fea

Copy link
Contributor

@jacobkeeler jacobkeeler left a comment

Choose a reason for hiding this comment

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

@AKalinich-Luxoft it looks like this PR might need to be rethought a bit with the changes made in #366

@AKalinich-Luxoft
Copy link
Contributor Author

@AKalinich-Luxoft it looks like this PR might need to be rethought a bit with the changes made in #366

@jacobkeeler after merge of #366 as I can see issues #313 and #339 are not reproducible on develop branch. So current PR after the conflicts resolution looks like just a refactoring of existing code. It's not a big deal to merge it now, however I recommend to do that as it makes proprietary code much cleaner and better documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants