-
Notifications
You must be signed in to change notification settings - Fork 11
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 explicit option to disable thresholds from CLI #329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed the expected behavior by building and testing locally. There is room for additional documentation to be added somewhere in the docs
directory...but that may already be in progress by @furi0us333 in the brad/docs_v2
branch.
LGTM.
Previously, when setting the project thresholds from the CLI, a value of `0` would automatically disable the specified threshold. This does not correctly represent the backend configuration, since some thresholds cannot be disabled and it is possible to have a threshold enabled with a value of `0`. To address both of these issues, an explicit `Disabled` option has been added to the interactive `set-thresholds` command, which allows controlling the `active` state of the threshold independently from its value. To ensure that the "Total Project" threshold is not disabled, a special case has been added which will inform the user that a value must be set for this threshold. Closes #288.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the threshold names and whether or not those thresholds can be disabled be coming from a dependency or an API call? It seems like they mean something somewhere in the backend. Otherwise LGTM.
Just for the future it would probably make sense to have some way to retrieve available thresholds and get information about the ability to enable/disable them. But since that hasn't been done before and knowledge about these thresholds is just hardcoded, I don't think it's justified to explore that in this PR. |
Previously, when setting the project thresholds from the CLI, a value of
0
would automatically disable the specified threshold. This does notcorrectly represent the backend configuration, since some thresholds
cannot be disabled and it is possible to have a threshold enabled with a
value of
0
.To address both of these issues, an explicit
Disabled
option has beenadded to the interactive
set-thresholds
command, which allowscontrolling the
active
state of the threshold independently from itsvalue.
To ensure that the "Total Project" threshold is not disabled, a special
case has been added which will inform the user that a value must be set
for this threshold.
Closes #288.