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

SWATCH-1934: Remove InstanceReportSort enum #2873

Merged
merged 1 commit into from Jan 8, 2024
Merged

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented Dec 7, 2023

Jira issue: SWATCH-1934

Description

My proposal here is to separate the map INSTANCE_SORT_PARAM_MAPPING into two collections:

  • FIELD_SORT_PARAM_MAPPING: to map the instance fields to the actual columns
  • METRICS_TO_SORT: auto-populated from metrics configuration at startup

Then, in https://github.com/RedHatInsights/rhsm-subscriptions/compare/jcarvaja/SWATCH-1934?expand=1#diff-90f2fbedf15069ad6f8fa3491fd724676933889502bd4cab8bcf99a5c4010982R261 the logic to select the sort has changed to use one or the another.

Testing

No functional change here. Only regression testing.

@Sgitario Sgitario added the QE Unneeded Pull request does not need QE approval label Dec 7, 2023
@kahowell
Copy link
Contributor

kahowell commented Dec 8, 2023

/retest

1 similar comment
@lindseyburnett
Copy link
Contributor

/retest

@lindseyburnett lindseyburnett self-assigned this Dec 14, 2023
@lindseyburnett lindseyburnett added QE Pull request should be approved by QE before merge and removed QE Unneeded Pull request does not need QE approval labels Jan 4, 2024
Copy link
Contributor

@lindseyburnett lindseyburnett left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I want to run the change by QE though to make sure we're not inadvertently dropping any generated test cases that may have been tied to the enumeration options. I added the QE label to this and the JIRA card and dropped a message in their slack channel.

@Aurobinda55
Copy link

/retest

@Aurobinda55 Aurobinda55 self-requested a review January 8, 2024 05:46
@Aurobinda55
Copy link

/retest

1 similar comment
@Aurobinda55
Copy link

/retest

@Aurobinda55
Copy link

@Sgitario Changes look good to me and no test failures on EE. I have created a PR to modify the iqe api spec ((https://gitlab.cee.redhat.com/insights-qe/iqe-rhsm-subscriptions-plugin/-/merge_requests/539)) . You can merge the PR and I will run the regression once the codes are in stage.

@Sgitario
Copy link
Contributor Author

Sgitario commented Jan 8, 2024

@Sgitario Changes look good to me and no test failures on EE. I have created a PR to modify the iqe api spec ((https://gitlab.cee.redhat.com/insights-qe/iqe-rhsm-subscriptions-plugin/-/merge_requests/539)) . You can merge the PR and I will run the regression once the codes are in stage.

Many thanks!

@Sgitario Sgitario merged commit 7ed5196 into main Jan 8, 2024
9 checks passed
@Sgitario Sgitario deleted the jcarvaja/SWATCH-1934 branch January 8, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QE Pull request should be approved by QE before merge
Projects
None yet
4 participants