opendatahub-io / model-registry-operator

Apache License 2.0
3 stars 17 forks source link

[Proposals] Personal take on improvements #136

Open Al-Pragliola opened 4 days ago

Al-Pragliola commented 4 days ago

Make use of Viper

Viper is setup in internal/controller/defaults.go

func init() {
    // init viper for config env variables
    viper.AutomaticEnv()
}

we can make use of it when taking configuration parameters from environment variables.

Switch from template files to actual entity types

I see the nice pattern of using template files to create different resources from the controller, but in my opinion it can be a double edged sword because we use the yaml package to map the templates to the kubernetes resource types and by default go if there's a mismatch with fields just sets the default value for that type without raising an error, creating a possible bug that is quite difficult to catch. If we use the go types to create the resources we lose the "nice to read" template but we have a 1:1 map from type to kubernetes applied resource.

Split large files

We should try to split the files that are close to 1k~ lines into several smaller files (https://google.github.io/styleguide/go/best-practices.html#package-size) to increase readability and reduce cognitive load.

Add more logs

We should add more logs to make debugging easier

Add linting

We should add linting steps, so to have and external help to avoid common mistakes and improve code quality

E2e tests ( envtest )

We should add e2e/envtest tests to avoid regressions and check that the operator is doing what is supposed to do

tarilabs commented 1 day ago

thank you @Al-Pragliola , I like especially about the e2e testing, I can see there are some already, but if those could be extended also to cover for non-regression in some "use-cases" or scenarios, I believe would be awesome.

rareddy commented 1 day ago

on template file usage I thought this was recommended pattern by operator SDK group to reduce the development time.

I also remember we advocated for linting from beginning. did we only use in rest server ? :)

+100 more e2e tests