Enable index and container sync options by default
migmartri opened this issue · 9 comments
Currently, we have two optional properties that are disabled by default that we could enable to get the best out of the box experience.
useChartsIndex
This one could be enabled by default as long as we keep the current behavior which is to enhance the UX not making it a requirement in any case by falling back to the charts: filter.
relocateContainerImages
The reason we left this option to false was so we do not change previous behavior of the tool. But there are other methods to communicate behavioral changes such as cutting a new release and so on.
WDYT?
cc/ @tompizmor @jotadrilo
The reason we left this option to false was so we do not change previous behavior of the tool. But there are other methods to communicate behavioral changes such as cutting a new release and so on.
Can you add more details here? I do not fully understand the alternatives.
I am OK enabling useChartsIndex by default, and even removing the option.
Thanks
I am OK enabling useChartsIndex by default, and even removing the option.
The only reason to leave this option is so an user can disable it if she knows that the repository will never have an index. It's just an optimization, probably unnecessary tbh
Can you add more details here? I do not fully understand the alternatives.
The initial reason for changing it was so charts-syncer behavior does not change under an existing user's feet. Meaning that a new version of the tool could starts doing something else (pushing images) when the user might already have a manual flow for that.
That said, I do not think it's a big deal because
- The tool does not auto-update and the user will need to upgrade their version of charts-syncer manually
- We are still releasing development versions 0.x so breaking changes are expected
- If we want to communicate the behavioral change clearly we could cut a 1.x release, although I am not sure if we are ready yet, and technically we do not need to do it because of the previous point.
I am probably missing more details because I foresee problems:
- What if the target registry is not an OCI-compliant registry? I do understand container images and chart helm templates can co-exist only if the target registry is OCI-compliant (you cannot push containers to a helm classic repository).
- What if there are users using the current behavior? (we have a test syncing some repositories to a local folder internally)
- What if I just don't want to relocate containers? I understand we will leave the flag there so we can set it to false?
What if the target registry is not an OCI-compliant registry? I do understand container images and chart helm templates can co-exist only if the target registry is OCI-compliant (you cannot push containers to a helm classic repository).
They don't have to coexist. You can sync the chart tarball to any repo and the container images to other.At #134 Migue added support to specify container credentials.
What if there are users using the current behavior? (we have a test syncing some repositories to a local folder internally)
Then they will keep the same behavior until they upgrade the tool. In that case yes, they probably will need to update the config file. But as Migue said, the released are tagged.
What if I just don't want to relocate containers? I understand we will leave the flag there so we can set it to false?
Yes, we could still set the option as false.
I understand.
Then I would just make sure we add some breaking changes notes to the release.
Good points
What if the target registry is not an OCI-compliant registry? I do understand container images and chart helm templates can co-exist only if the target registry is OCI-compliant (you cannot push containers to a helm classic repository).
Technically you are providing two target entities, chart repository (target.repo) and container registry (target.containerRegistry + target.containes.auth). So yes, in order for this to work it will require that the user makes sure that the target container credentials are set, target.containerRegistry has always been there.
So what will happen if an user updates the version of charts-syncer is that the container images push will fail and the user will need to either set the relocateContainerImages option to false or fix the authN https://github.com/bitnami-labs/charts-syncer#manage-credentials
What if I just don't want to relocate containers? I understand we will leave the flag there so we can set it to false?
That's right, the flag will still be there
I've been giving it a crack to this issue by setting the defaults using viper like for example viper.SetDefault(optRelocateContainerImages, true) the problem is that the current approach gets quite messy since we have two mechanisms to load configuration, via yaml->json->pbjson.marshall and via viper (config, env variables and now defaults) and making sure the state is consistent is hard.
Ideally we should move to one mechanism only, either using viper or using manual config marshalling + manual env variables. I tried the former but viper.Unmarshall doesn't work with protobuff definitions because among other things the oneof we use so it seems we have two options
1 - We replace the API definition from using protobuffers to regular structs so we can fully move to viper.
2 - We remove viper altogether and manually implement defaults
The first option will require quite more work since we will loose safe accessors such as GetTarget(), the second option might be easier to do but we will loose potential features we can leverage in the future such as automatic env variables.
I am personally starting to lean towards 2, if in the future we need some of the features of viper we can tackle 1.
There is a 3rd option which is to implement defaults manually as we are doing today in some cases, but this will just extend the pain of maintaining two different systems to do the same.
WDYT?
Since the configuration is not meant to be serialized and transferred between applications, I am OK relying on viper only.
I am having mixed feelings about enabling relocateContainerImages by default.
I noticed that the tool output when the charts do not contain the files can be confusing since it returns errors when maybe we should return warnings and some instructions on what's going on and how to fix it (adding those files)
The readme file can be also confusing because it maintains information of two operational modes, I would say we need a cleanup before adding this feature by default.
Would you be ok if I only enable the index by default for now?