canonical / kafka-operator

Kafka VM operator
Apache License 2.0
6 stars 12 forks source link

[DPE-3780] Set workload version in install hook #182

Closed Batalex closed 3 months ago

Batalex commented 3 months ago

I'd appreciate feedback on:

New interrogation: do we need to make sure that only the leader can set the workload version? I'd like to say that we do not care, but I have some doubt about an upgrade involving multiple units

deusebio commented 3 months ago

do we keep the workload setter statement in the install hook? The one in the config changed hook would be sufficient, but as a reader, I would expect this hook to run this action

Interesting, I would have said that having it in the install hook was sufficient :joy: . But honestly, I don't think this there is any drawback on having it in both, to be safer and clearer. +1 for on both

any idea on how we can run an integration test without overmocking everything? IMO the unit test is already a low-value pass through test

I can take care of integration testing, using this functionality, when implementing DPE-3857. So I would keep things as-is

Batalex commented 3 months ago

As per my discussion with Marc, I'll change the implementation to get the version from the actual workload instead of asking the snap client.