Closed mtyazici closed 3 years ago
@leszko Thanks for the review. I will try to address your questions.
In general, our development process should be:
1.You create TDD 2.TDD is reviewed, the solution is discussed, and the TDD is approved 3.Then we implement and send the PR
Otherwise, you may spend a lot of time on the implementation while we actually don't have an agreement on what we want to implement.
I thought we had decided on the implementation when we said we would use one bundle file. From now on, I will make sure to start implementation after the TDD is approved.
Do we want to have ONLY bundle.yaml? Or do we want to have both, the files we had and the new bundle.yaml. The answer to this question is not that simple, because I see different operators do it in different ways. This requires research. If we decide to do both, then you don't need to change all these release files in this PR.
I thought keeping the files that we merged into bundle would be redundant if we don't address them anywhere. When I updated the READMEs I also deleted those files as we were not using them anywhere. However, I can look at what other projects are doing, then we can decide on what to do.
I see sometimes bundle files are enriched with openAPIV3Schema. Do we want/need it?
openAPIV3Schema is used for validating the created resource at the Kubernetes API. I think it is valuable to have the schema defined. But of course we can discuss its need.
Sometimes bundle files are actually called all-in-one.yaml which is very explicit. Do we want to call it that way?
As far as I saw, only a few operators' bundle file are called all-in-one.yaml
. I saw others using bundle.yaml
and operator.yaml
. I don't think we should change the bundle file's name.
Should Hazelcast RBAC be included in the bundle file? What do other operators do in similar situations?
In my research I haven't come across with bundle files that include RBAC for operator resources. However, those bundle files contained a lot CRDs. Currently, our operator only monitors one custom resource, so I think it is reasonable to also add the RBAC for Hazelcast resource type.
I found the Spark project that creates the operator resource RBAC as the operator is deployed, though they are using Helm Chart instead of bundle file. The similarity between our and their operator is that both operators only watch one custom resource. So, it would also make sense for us to add RBAC for Hazelcast resource in the bundle file.
Changes
For hazelcast-operator,
hazelcastcluster.crd.yaml
,operator-rbac.yaml
andoperator.yaml
are merged into onebundle.yaml
file.For hazelcast-enterprise-operator two bundle files are created:
bundle.yaml
andbundle-rhel.yaml
. The only difference between them isbundle.yaml
containsoperator-docker-hub.yaml
whilebundle-rhel.yaml
containsoperator-rhel.yaml
.The release workflows are also affected by this change.
For Docker Hub release, changes were minimal. Only file names are changed.
For Red Hat release, on top of file name changes test scripts had to be changed too. The command
oc secrets link hazelcast-enterprise-operator pull-secret --for=pull
has to be run between operator service account creation and operator deployment. However with thebundle.yaml
, there is no such possibility. So I chose to create a service account which is a clone of the one inbundle-rhel.yaml
that is run before the aforementioned command. The order looks like the followingBundle file also caused another problem when deleting the created resources. With the old script, Hazelcast CRD wasn't deleted after the test. Deleting
bundle-rhel.yaml
causes the CRD to be deleted but deletion does not complete successfully. So, inclean-up.sh
, each resource created inbundle-rhel.yaml
is deleted individually except the CRD.