cnquery icon indicating copy to clipboard operation
cnquery copied to clipboard

πŸ› fix core `asset.labels` + ✨ new `asset.platformMetadata`

Open afiune opened this issue 1 year ago β€’ 6 comments

The labels of an asset come from the asset itself, not the platform, but as some point we made a mistake to use the platform labels, this PR is the first step to fix this issue.

In this PR we are:

  • Introducing a new field to the proto message asset.Platform.Metadata with the goal to give more clarity and avoid confusions with asset.Labels
  • We are deprecating asset.Platform.Labels in favor of asset.Platform.Metadata (this field will be remove in v12)
  • We are introducing a new asset.platformMetadata to the asset MQL schema, this new field will have the asset.Platform.Metadata
  • We are fixing the asset.labels MQL resource to have the actual asset labels but, we are going to merge this field with the platform labels for backwards compatibility

After merging this PR, we will start migrating the use of asset.Platform.Labels to asset.Platform.Metadata, so that in v12 we can remove `asset.Platform.Labels.

afiune avatar Feb 14 '25 21:02 afiune

Test Results

3β€ˆ369 tests  +15   3β€ˆ365 βœ… +15   1m 38s ⏱️ -6s β€‡β€ˆ392 suites + 1β€‚β€ƒβ€ƒβ€‡β€ˆβ€‡β€‡4 πŸ’€ ± 0  β€‡β€ˆβ€‡30 files   ± 0β€‚β€ƒβ€ƒβ€‡β€ˆβ€‡β€‡0 ❌ ± 0 

Results for commit 7ae781ef. ± Comparison against base commit 03ab2213.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Feb 14 '25 21:02 github-actions[bot]

Why did we use Platform.Labels before and what change does it make to switch to Asset.Labels? Are there different code paths that set these across different providers? I'm worried that with both fields existing we might run into similar problems again.

arlimus avatar Feb 14 '25 21:02 arlimus

I do some extra testing but this is what I discussed with @afiune as a solution. It stuck me that we use asset.Annotations but not asset.Labels. I am 99% sure the labels from platform are actually incorrect.

chris-rock avatar Feb 14 '25 21:02 chris-rock

@chris-rock I asked @afiune to investigate the solution, so we make sure it doesn't break things. We shouldn't have both structures because it can lead to bugs like this one :)

arlimus avatar Feb 14 '25 21:02 arlimus

Thank you both, I agree with @arlimus, if we don't use one, we should get rid of it.

I will do some digging and report back any findings.

afiune avatar Feb 15 '25 03:02 afiune

@chris-rock @arlimus I found a few places where we are using asset.platform.labels, I can update the tests but I want to check with you both if this change is heading in the right direction, or not.

afiune avatar Feb 17 '25 06:02 afiune