Why don't you just name it toConfigMap / getToConfigMap / setToConfigMap?

I was trying to provide a more descriptive method name just to make the code more readable, happy to change it though if you think it's better to stay closer to the actual property name

I personally find it much better when the names follow the API names. It makes it easiest to align the Java and the CRD models, it makes lot less confusion when checking some YAML against the Java code etc. I also think we usually don't map the property like this unless it conflicts with some Java keywords. So I personally would change this.

Alternatively, I personally would be also fine if we change the API against the proposal and name the field simply configMap. It might make the name less weird and possibly allow to reuse the same class for both listing and setting the offsets.

I've switched it to setToConfigMap and getToConfigMap

I guess I can accept that we have the annotations enum in the api. But it probably should have only the valid values and not none or unknown, or? Why is that needed?

I added them to use when processing the annotation in the code, but in reality I'm only using none as the default when fetching the annotation so I'll remove unknown. I placed this class in api as I figured it's part of the API, but it's actually only used from the cluster-operator module, so should I actually move it there?

I was wondering if it should be in the api. I'm personally not a big fan of it. But we already do have KafkaRebalanceAnnotation as an example of this being in the api module before. So I felt like I should not insist on this being moved (funny enough, it also includes the same options you have there which I did not notice yesterday 🙄).

I think the logic of having it in the API is that users might use it to set the annotation programmatically. But then it would be better if it contained the real values that the user is expected to use and not the none and unknown values.

So, I don't know ... I would do it differently. But it does not feel fair to insist you change it if you are happy with it 🤷.

That's a fair point Jakub instead. I was thinking about the same (while working on auto-rebalancing) but while i.e. refresh, approve, stop are needed by the users, the none and unknown are there just as internal values for the rebalance logic. Maybe Kate could address this here (now) and we should do the same on the rebalance operator side.

@ppatierno I've removed unknown since I don't use that internally. When you say "Maybe Kate could address this here (now)" do you mean moving the class to cluster-operator module?

unknown was removed but none is still there which is not a valid annotation, it's just the absence of it.
IIRC the idea of having these annotation in the api module was because of providing enums to a user who is developing an automation on handling a KafkaRebalance resource, so instead of writing a string, they can use our enum. Of course, having none (and unknown) brings confusion. So or we put these none somewhere else leaving the enum in the api module or we just remove it from the api module (and we should do the same for the rebalancing operator) leaving the user to use the api module but then just strings when it comes to apply these annotations.

I've moved it to io.strimzi.operator.cluster.operator.resource in the cluster-operator module, if you're both happy with that I'll mark this thread as resolved.

You mean the whole annotation class? Would io.strimzi.operator.cluster.model be better? Or at least io.strimzi.operator.cluster.operator.assembly?

Agree on the model package.

The names with Annotation in singular sound strange if you then actually use multiple of them. Maybe we can try to find some more neutral names?

What about getConnectorOffsetsAnnotation -> getConnectorOffsetsOperation or getConnectorOffsetsAction?
And removeConnectorOffsetsAnnotation -> `removeConnectorOffsetsAnnotations?

We could even rename the enum KafkaConnectorOffsetsAnnotation to KafkaConnectorOffsetsOperation

Yeah, I think calling it operation or action would make it a bit more clear. Not sure I would change the KafkaConnectorOffsetsAnnotation name as that is the enum with the possible values, or? So that makes sense to and with Annotation.

I've renamed them

Do we have some Enum instead of this magic constant? I assume you can use io.strimzi.api.kafka.model.common.ConnectorState? :) The same applies in previous parts.

The tricky part here is the ConnectorState enum values are based on the CRD field which is lowercase, whereas the state returned from the Connect API is upper case. @scholzj @mimaison what do you think of adding a fromStatus method to io.strimzi.api.kafka.model.common.ConnectorState that we can call directly here to convert the state to the ConnectorState enum? I noticed that the code styles dislikes using toUpperCase and toLowerCase

The tricky part here is the ConnectorState enum values are based on the CRD field which is lowercase, whereas the state returned from the Connect API is upper case. @scholzj @mimaison what do you think of adding a fromStatus method to io.strimzi.api.kafka.model.common.ConnectorState that we can call directly here to convert the state to the ConnectorState enum?

I'm not sure I would add special method like that to the api module. But maybe you can simply convert it to lowercase in the forValue method? We do that in other places ... ProcessRoles, NodeAddressType etc.

I noticed that the code styles dislikes using toUpperCase and toLowerCase

You should be able to use them, but you should specify the locale:

toLowerCase(Locale.ENGLISH)

Ah I misunderstood the code style error then, I've updated it to do the toLowerCase

That's a fair point Jakub instead. I was thinking about the same (while working on auto-rebalancing) but while i.e. refresh, approve, stop are needed by the users, the none and unknown are there just as internal values for the rebalance logic. Maybe Kate could address this here (now) and we should do the same on the rebalance operator side.

could be onSuccess and onFailure be used to make it more readable? No need to change just an idea.

Using onSuccess and onFailure doesn't work because in the failure case we still want to return a succeeded future, so we need to pass a mapper, whereas onFailure takes a handler, so the method would still return a failed future. Similarly for onSuccess we have to map the result, not just register a handler.

I could change it to use map and otherwise?

        return apiClient.getConnectorOffsets(reconciliation, host, port, connectorName)
                .compose(offsets -> generateListOffsetsConfigMap(configMapName, connectorName, resource, offsets))
                .compose(configMap -> configMapOperations.reconcile(reconciliation, resource.getMetadata().getNamespace(), configMapName, configMap))
                .compose(v -> removeConnectorOffsetsAnnotations(reconciliation, resource))
                .map(v -> conditions)
                .otherwise(throwable -> {
                    // Don't fail reconciliation on error from listing offsets - add a warning and repeat list on next reconcile
                    String message = "Encountered error listing connector offsets. " + throwable.getMessage();
                    LOGGER.warnCr(reconciliation, message);
                    conditions.add(StatusUtils.buildWarningCondition("ListOffsets", message));
                    return conditions;
                });

I like it, I think it's more readable but I will leave the final call to you.

I've updated it

Ditto as above.