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

APIv4 - remove unnecessary field from System::check #22748

Merged
merged 1 commit into from
Feb 22, 2022

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 10, 2022

Overview

Following on #22730, this removes the 'severity' field from the System::check api
because it's redundant with 'severity_id:name', and does not appear to be used anywhere.

Before

Redundant field exists. Not used in core. Probably not used outside of core.

After

Removed.

Following on civicrm#22730, this removes the 'severity' field from the System::check api
because it's redundant with 'severity_id:name', and does not appear to be used anywhere.
@civibot
Copy link

civibot bot commented Feb 10, 2022

(Standard links)

@civibot civibot bot added the master label Feb 10, 2022
@eileenmcnaughton
Copy link
Contributor

@colemanw is it possible it is being used from outside of core?

@colemanw
Copy link
Member Author

@colemanw is it possible it is being used from outside of core?

I seriously doubt it. I know that @MegaphoneJon has an extension which uses this API but I'm pretty sure he's using v3.

@totten
Copy link
Member

totten commented Feb 19, 2022

Core uses severity, but AFAICS it only does APIv3.

ang/crmStatusPage.js:          return crmApi('System', 'check', {sequential: 1, options: {limit: 0, sort: 'severity_id DESC'}});
ang/crmStatusPage/StatusPageCtrl.js:        apiCalls = (apiCalls || []).concat([['System', 'check', {sequential: 1, options: {limit: 0, sort: 'severity_id DESC'}}]]);
ang/crmStatusPage/StatusPageCtrl.js:            ignore_severity: visible ? 0 : status.severity,
ang/crmStatusPage/StatusPageCtrl.js:          return s.is_visible == visibility && s.severity_id >= 2;
ang/crmStatusPage/StatusPage.html:      <h3 class="crm-severity-{{status.severity}}">

I grepped universe for system.*check and found a couple references to the v3 variant but not v4. Some more interested parties might be @agh1 @mlutfy.

I think you're probably right that folks polling for status-checks are more likely to be hitting APIv3... because you'd want some external entry-point, and we've historically had better tooling+docs for v3 access (drush/wp-cli/rest/etc).

Let's wait until Tuesday to see Jon/Andie/Mathieu have any issues with dropping v4's severity field (in favor of severity_id:*). If none, then go ahead and merge.

@totten totten added the merge ready PR will be merged after a few days if there are no objections label Feb 19, 2022
@MegaphoneJon
Copy link
Contributor

Sorry - I was on vacation last week. No, I don't use this API, I use Andie's Nagios plugin which uses API3 System.check.

@colemanw colemanw merged commit bd8f40c into civicrm:master Feb 22, 2022
@colemanw colemanw deleted the api4SystemCheck branch February 22, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants