-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable index and container sync options by default #132
Comments
Can you add more details here? I do not fully understand the alternatives. I am OK enabling |
Thanks
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
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
|
I am probably missing more details because I foresee problems:
|
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.
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.
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
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, 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
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 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 1 - We replace the API definition from using protobuffers to regular structs so we can fully move to viper. The first option will require quite more work since we will loose safe accessors such as 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 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? |
Currently, we have two optional properties that are disabled by default that we could enable to get the best out of the box experience.
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.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 text was updated successfully, but these errors were encountered: