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-5565] - dont match on empty resource ids (OCP/AWS managed tables) #5425

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

lcouzens
Copy link
Contributor

Jira Ticket

COST-5565

Description

This change will filter out OCP resources with an empty resource_id ''

Testing

  1. Checkout Branch
  2. Restart Koku
  3. Hit endpoint or launch shell
    1. You should see ...
  4. Do more things...

Release Notes

  • proposed release note
* [COST-5565](https://issues.redhat.com/browse/COST-5565) Fix OCP/AWS managed table resource matching on `''`

@lcouzens lcouzens added the hot-fix-smoke-tests pr_check label to run minimal smoke tests for fast moving bug-fix label Dec 12, 2024
@lcouzens lcouzens requested review from a team as code owners December 12, 2024 11:25
@lcouzens lcouzens changed the title [COST-5565] - dont match empty resource ids [COST-5565] - dont match on empty resource ids (OCP/AWS managed tables) Dec 12, 2024
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5425     +/-   ##
=======================================
- Coverage   94.2%   94.1%   -0.0%     
=======================================
  Files        371     371             
  Lines      31542   31542             
  Branches    3378    3378             
=======================================
- Hits       29698   29696      -2     
- Misses      1198    1199      +1     
- Partials     646     647      +1     

@@ -105,6 +106,8 @@ cte_array_agg_volumes AS (
SELECT DISTINCT persistentvolume, csi_volume_handle
FROM hive.{{schema | sqlsafe}}.openshift_storage_usage_line_items_daily
WHERE source = {{ocp_source_uuid}}
AND persistentvolume != ''
AND csi_volume_handle != ''
Copy link
Contributor

Choose a reason for hiding this comment

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

If customers have not updated their operator they will not have the csi_volume_handle. So, we need to account for the case where a persistentvolume has been populated, but the csi_volume_handle has not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered if that might be a case!! Thanks. I originally just had the check on persistentvolume but wasnt sure if that would be enough. Based on what your saying I think that would be the better approach here.

@myersCody
Copy link
Contributor

We do still need to prevent an empty csi_volume handle string on the condition though or it will always return true. Give this a try, I have not ran it so it may need some tweaking:

OR (volumes.csi_volume_handle != '' AND substr(resource_names.lineitem_resourceid, -length(volumes.csi_volume_handle)) = volumes.csi_volume_handle)

@lcouzens lcouzens enabled auto-merge (squash) December 12, 2024 18:33
@lcouzens lcouzens merged commit 94d2544 into main Dec 13, 2024
14 checks passed
@lcouzens lcouzens deleted the COST-5565-fix-empty-resource_id branch December 13, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hot-fix-smoke-tests pr_check label to run minimal smoke tests for fast moving bug-fix smokes-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants