scylladb / scylla-operator

The Kubernetes Operator for ScyllaDB
https://operator.docs.scylladb.com/
Apache License 2.0
324 stars 159 forks source link

Add an e2e suite covering manager API conversion to test unhandled properties #1870

Open rzetelskik opened 3 months ago

rzetelskik commented 3 months ago

What should the feature do?

We're using mapstructure to map properties of manager tasks taken from manager state to our status.

https://github.com/scylladb/scylla-operator/blob/7d241f64cc00fd81c08ea6ae2e4e6dd3ef4f01b5/pkg/controller/manager/types_old.go#L91

At the moment there's no way for us to know if we're missing any properties, so we should at least log a warning if any of the source properties were not handled by the converting func.

/kind feature /priority important-longterm

What is the use case behind this feature?

ditto

Anything else we need to know?

No response

tnozicka commented 3 months ago

If a new field is added and we don't use it, I don't think a warning is appropriate. Assuming API have some backwards compatibility, this is a normal state. Library wise it could be handle with strict decoding option or returning extra fields and let the caller to decide.

Maybe this would be useful to check in a unit test for our dev signal but not a warning in production logs.

rzetelskik commented 3 months ago

Maybe this would be useful to check in a unit test for our dev signal but not a warning in production logs.

Our unit tests don't issue actual calls to scylla manager, so we only get whatever properties we put ourselves in the mock manager state.

Library wise it could be handle with strict decoding option or returning extra fields and let the caller to decide.

Yup, that's how it would work. But we need a way to learn about this, the only other way we have is to inspect manager release notes and/or code changes atm, in which case it's easy to miss. Otherwise we can just take a different approach - only add the properties whenever we find them necessary - so most likely when someone files an issue.

tnozicka commented 3 months ago

I don't mind having the check but I don't want to bother users with this while there is nothing they can do about it. There are ways to setup an e2e test that fails on manager decoding modulo "allowed list", so we find out early and can be labeled so it runs only in our CI.

rzetelskik commented 3 months ago

Updated the issue title

rzetelskik commented 3 months ago

/priority backlog