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

[COST-5137] Initial managed summary tasks rework #5469

Merged
merged 13 commits into from
Feb 6, 2025
Merged

Conversation

lcouzens
Copy link
Contributor

@lcouzens lcouzens commented Jan 28, 2025

Jira Ticket

COST-5137

Description

This change will move our ocp on cloud update summary tables logic to its own function and enable us to trigger it subtly differently for Manged tables vs Standard ingest flows.

Test 1

  1. Checkout Branch
  2. Restart Koku
  3. ingest data like normal (Old way)
  4. Everything should function correctly (no regressions)

Test 2

  1. Checkout Branch
  2. Restart Koku
  3. Enable the following unleash flags is_managed_ocp_cloud_processing_enabled and is_managed_ocp_cloud_summary_enabled (the summary flag additionally has a check on provider type too, which would also need to be enabled if your doing it via an unleash client)
  4. Ingest ocp on cloud data.
  5. Currently should see an error message of managed_####### table does not exist (requires Cody's [COST-5137] Azure managed summary #5475)
  6. With Cody's PR everything should be populated correctly

Release Notes

  • proposed release note
* [COST-5137](https://issues.redhat.com/browse/COST-5137) Adds new trigger for managed tables summary

@@ -583,77 +642,22 @@ def update_summary_tables( # noqa: C901
set_summary_timestamp(ManifestState.FAILED, manifest_id)
raise ex

if provider_type != Provider.PROVIDER_OCP:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

586 to 597 here is redundant code now that we always run cost model updates regardless of a cost model existing now. Also tweaking line 655 below should make this simpler.

@lcouzens lcouzens added the smoke-tests pr_check will build the image and run minimal required smokes label Jan 29, 2025
@lcouzens lcouzens force-pushed the COST-5137-summary-task branch from 99f65c8 to d2971c8 Compare January 31, 2025 11:43
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 89.18919% with 8 lines in your changes missing coverage. Please review.

Project coverage is 94.1%. Comparing base (fa5bb8a) to head (faebb65).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5469     +/-   ##
=======================================
- Coverage   94.1%   94.1%   -0.0%     
=======================================
  Files        371     371             
  Lines      31610   31644     +34     
  Branches    3380    3388      +8     
=======================================
+ Hits       29743   29766     +23     
- Misses      1214    1220      +6     
- Partials     653     658      +5     

@lcouzens lcouzens marked this pull request as ready for review February 3, 2025 15:32
@lcouzens lcouzens requested review from a team as code owners February 3, 2025 15:32
@lcouzens lcouzens merged commit 43da108 into main Feb 6, 2025
14 checks passed
@lcouzens lcouzens deleted the COST-5137-summary-task branch February 6, 2025 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smoke-tests pr_check will build the image and run minimal required smokes smokes-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants