Skip to content

Ensure the FBC --> entity translation for bundle properties is not lossy #258

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

Closed
everettraven opened this issue Jun 8, 2023 · 6 comments · Fixed by #427
Closed

Ensure the FBC --> entity translation for bundle properties is not lossy #258

everettraven opened this issue Jun 8, 2023 · 6 comments · Fixed by #427

Comments

@everettraven
Copy link
Contributor

Currently the translation for catalog contents --> entity is lossy when it comes to bundle properties. As we can see from

for _, prop := range bundle.Spec.Properties {
switch prop.Type {
case property.TypePackage:
// this is already a json marshalled object, so it doesn't need to be marshalled
// like the other ones
props[property.TypePackage] = string(prop.Value)
}
}
we are only passing through bundle properties that are of type olm.package and therefore losing any other bundle property information.

We should update this to ensure that all bundle properties are being forwarded through during the entity translation so they can be used properly.

@joelanford
Copy link
Member

@perdasilva This seems like a rough edge of the translation from FBC to input.Entity. I know the Properties []property.Property field in FBC is problematic, but it seems more problematic that we don't have an opaque canonical translation function.

Another option would be to have input.Entity change its properties field to match (and maybe translate internally)?

Either way, I think we need to smooth out this edge.

@joelanford
Copy link
Member

Also, not saying it has to be a deppy concern. This is an OLM-specific thing, so if that translation belongs in operator-controller, that's a-OK!

@m1kola
Copy link
Member

m1kola commented Jun 9, 2023

Today properties on a Deppy input.Entity are defined as a map of strings. But in operator-controller I think we can pack anything we want into this map in any format we want: we don’t have to change Deppy for operator-controller to understand repeated properties as Deppy itself is not reading properties (but we can consider change to avoid weird marshalling on controller side). And BundleMetadataSpec already has properties as a slice. So to me it looks like the ball is entirely on the operator-controller side.

@perdasilva
Copy link
Contributor

+1 to @m1kola: I think this fix should go in op-controller. Entities are dumb by design. Though, they might go away. I'd say the right way to fix this would be to just laydown a json string in the property - and copy all of the properties over.

@m1kola
Copy link
Member

m1kola commented Sep 21, 2023

In #413 we got rid of entity source & entity code and now have access to all bundle props, but there is currently an issue on read where we lose some values. Once #427 gets merged, I think we can consider this done.

@m1kola
Copy link
Member

m1kola commented Sep 21, 2023

#427 is merged now. For some reason GH didn't close the issue, so I'm doing it manually.

@m1kola m1kola closed this as completed Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants