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 freely setting project thresholds #25

Merged
merged 1 commit into from
Apr 29, 2022
Merged

Allow freely setting project thresholds #25

merged 1 commit into from
Apr 29, 2022

Conversation

cd-work
Copy link
Contributor

@cd-work cd-work commented Apr 28, 2022

Previously the active field of the thresholds struct was populated by
checking whether the threshold was above 0 or not. However the
Total Project Threshold can never be disabled and it is possible for
other thresholds to be at 0 without being disabled too.

To ensure a more accurate representation of the backend state, the
set_threshold method instead takes a Threshold instance which can be
populated fully by the consumer without any internal restrictions.

See phylum-dev/cli#288.

@cd-work
Copy link
Contributor Author

cd-work commented Apr 28, 2022

If a non-breaking change is preferred, it would also be possible to create a separate method. However I've opted to fix things rather than duplicating it.

Previously the `active` field of the thresholds struct was populated by
checking whether the `threshold` was above `0` or not. However the
`Total Project Threshold` can never be disabled and it is possible for
other thresholds to be at `0` without being disabled too.

To ensure a more accurate representation of the backend state, the
`set_threshold` method instead takes a `Threshold` instance which can be
populated fully by the consumer without any internal restrictions.

See #288.
Copy link
Contributor

@kylewillmon kylewillmon left a comment

Choose a reason for hiding this comment

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

Even though it's a breaking change, I agree with this.

I'm not sure that API even calls this function, but we should wait for someone from @phylum-dev/user-components to chime in just in case.

@cd-work cd-work requested review from a team and DanielJoyce and removed request for a team April 28, 2022 21:47
@DanielJoyce
Copy link
Contributor

This isn't called in api, but may be in CLI,

@cd-work
Copy link
Contributor Author

cd-work commented Apr 29, 2022

This isn't called in api, but may be in CLI

Yes, this is where it's called and what motivated this PR.

@cd-work cd-work merged commit 960c56a into development Apr 29, 2022
@cd-work cd-work deleted the thresholds branch April 29, 2022 20:37
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.

3 participants