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

Allow to control (again) the showPendingRetry setting #763

Merged
merged 8 commits into from Jul 26, 2019

Conversation

@mauroservienti
Copy link
Member

commented Jul 24, 2019

As stated in the documentation by tweaking the app.constants.js it should be possible to enable the "show pending retry" feature in ServicePulse. This was broken due to way Pulse was changed to introduce the ability to control che ServiceControl connection. This PR fixes the regression by re-enabling the ability to turn-on the "show pending retry" feature.

PoA

@WilliamBZA

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@mauroservienti What were the symptoms of if being broken? I'm in 2 minds about adding the setting to the config as a default false when a .constant('showPendingRetry', (window.defaultConfig.showPendingRetry || false)) would also work.

On the one hand:

  • It's easier for me to remember what the setting is called
  • It's less likely the setting will be forgotten in the future again because it's visible

On the other hand:

  • It's not a feature we really want to expose to users, by having it there they can see it.

On the middle-hand:

  • I don't really know what the concrete use case for the feature is. It has always felt to me like something that could be better solved.

So I guess I'm leaning towards adding the default values, I just need a little nudge to push me over.

@mauroservienti

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

Doco says "change .constant('showPendingRetry', false) to trueinapp.constants.jsbut theapp.constants.jsformat changed and that.constant('showPendingRetry', false)doesn't exist anymore and there is no place to add it inapp.constant.js. This means that there is no way to enable the feature.

I'm fine with not adding anything to app.constant.js and use your code:

.constant('showPendingRetry', (window.defaultConfig.showPendingRetry || false))

This means more changes to doco.

It's not a feature we really want to expose to users, by having it there they can see it.
I don't really know what the concrete use case for the feature is. It has always felt to me like something that could be better solved.

The feature is documented, this PR originates from a chat with a customer that upgraded to latest Pulse and failed to re-enable the feature.

mauroservienti added some commits Jul 25, 2019

@mauroservienti

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

@WilliamBZA updated.

mauroservienti added some commits Jul 25, 2019

Revert "Remove default value"
This reverts commit c7fe341.
Revert "Remove default value"
This reverts commit c7f5841.
@WilliamBZA
Copy link
Member

left a comment

👍 I'm convinced. The code is documented and arguably should not have been removed.

@mauroservienti

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

tested: works as expected.

@mauroservienti mauroservienti added this to the 1.20.4 milestone Jul 26, 2019

@mauroservienti mauroservienti merged commit b3220eb into master Jul 26, 2019

4 checks passed

Compile Finished TeamCity Build ServicePulse / 1. Compile : Running
Details
Test .NET Framework on Windows Finished TeamCity Build ServicePulse / 3.1 Test : Tests passed: 102
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@mauroservienti mauroservienti deleted the fix-show-pending-retries-regression branch Jul 26, 2019

@mauroservienti mauroservienti removed the request for review from WojcikMike Jul 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.