π fix core `asset.labels` + β¨ new `asset.platformMetadata`
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.Metadatawith the goal to give more clarity and avoid confusions withasset.Labels - We are deprecating
asset.Platform.Labelsin favor ofasset.Platform.Metadata(this field will be remove inv12) - We are introducing a new
asset.platformMetadatato theassetMQL schema, this new field will have theasset.Platform.Metadata - We are fixing the
asset.labelsMQL 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.
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.
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.
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 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 :)
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.
@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.