updatecli icon indicating copy to clipboard operation
updatecli copied to clipboard

Ability to globally define the `scms` block (v0.17+)

Open lemeurherve opened this issue 3 years ago • 7 comments

Is your feature request related to a problem? Looking at the new syntax of updatecli manifest for version 0.17.0 and above like in this PR, we can see the block scms is repeated word for word in every manifest.

Describe the solution you'd like I'd like the ability to define it globally so we don't have to repeat it on every manifest needing a pull request.

Describe alternatives you've considered N/A

Additional context N/A

lemeurherve avatar Dec 21 '21 14:12 lemeurherve

I have been thinking about this issue quite a lot recently and it will require some refactoring anyway. At the moment everything is defined per pipeline where a pipeline matches an updatecli configuration file.

I am thinking to have something like:

.draft.yaml

scms: # Moving the SCM configuration at this level
  scm;
    kind: github
pipelines:
  specDirectory: "./updatecli.d" # So we can read all configuration from a directory like we do now
  spec:
    - name: Bump updatecli file
      specFile: "updatecli.yaml" ## So we can reference existing updatecli configuration without having to copy them here
    - name: Bump go version
      spec: ## So we can insert a pipeline configuration here
        title: Bump golang version
        sources:
            ...
        conditions:
            ...
         targets:
            ...

Something that I couldn't really decide is if If we need uniq source/condition/targets or if we need uniq "pipeline"

olblak avatar Dec 22 '21 07:12 olblak

We might consider using viper for these, to allow specifying in a standard format either throug env vars, CLI flags or config files.

I vote for only reworking the SCM first as there is a need for it expressed by @lemeurherve in this issue (while having a top level wrapper config file could prove interesting but it not asked by a user for now)

dduportal avatar Dec 22 '21 07:12 dduportal

Even if it's an interesting idea that could solve the problem, I have concerns with the viper approach as it introduces a new mechanism for the configuration. What about having a new flag --base-config that we would use like updatecli diff --base-config base.yaml --config updatecli.d --values values.yaml and so we would do a deep merge between base.yaml and each of values under updatecli.d

base.yaml could contain any valid updatecli config.

We already have the ability to merge values files so we could have the same approach for config files as well. https://github.com/updatecli/updatecli/blob/b527434e4f72a3a6a8a6c6489bb375723c86d3e9/pkg/core/config/template.go#L148

Please note that the current function Merge is not a deep merge so some improvement needs to be done on that side

olblak avatar Dec 24 '21 07:12 olblak

I don't see any arguments against your proposal of a base config: it seems legit if it fixes the issue "As a end user, I don't want to maintain 1 copy of my SCM block for each pipeline, without changing my user experience".

I'm concerned by us, trying to re-invent a configuration mechanism with merge (that are note deep merge: which defeats the whole purpose of the configuration that could be overloaded): if we do not do deep merge, then we should allow any map/dictionnary in the manifests that could be defined on different locations, such as scm.

Switching to viper might not feel easy but it will immediatly underline the flaws of our configuration model and point at these flaws so we can fix it one time for all. But it could be interesting to specify the new "base config": you should not have to break the exisint configuration model: viper provides you with the attribute config, you don't have to care if it comes from an ENV var UPDATECLI_CONFIG, or from a config flag --config or from the attribute config: in the base config file (or all of them with merging).

dduportal avatar Dec 24 '21 07:12 dduportal

I am considering working on the current issue as It could simplify the autodiscovery manifest by not having to specify scm or action configuration.

Right now I see two ways to solve

1. New top level key "global" in existing manifest

We could have a new top level key named "global" such as

scms:
    default1:
        kind: git
        spec:
             ...
global:
    scms:
        default2:
            kind: git
            spec:
                 ...

Resources defines in a global section would be available to any pipeline as long as a pipeline do not specify a key such as "default2" in this example

2. New cli flag

Similarly to my idea explained in https://github.com/updatecli/updatecli/issues/430#issuecomment-1000703155 We could have a flag name "--gobal-config" or "--base-config", or "--default-global" that would work similarly to "--config" and would allow us to specify any Updatecli manifest as the default config.

Regarding the deep-merge topic, my guts tell me to avoid that rabbit hole but I don't have strong opinion

olblak avatar Dec 08 '22 12:12 olblak

I'm not sure about the 1. New top level key "global" in existing manifest. As a user, it seems confusing: which YAML manifest file holds the global? What if I got 2 different files? etc.

The 2nd one looks really promising!

dduportal avatar Dec 08 '22 16:12 dduportal

Thanks for your feedback, I am also more in favor of the second approach. I'll try to find some time to dig into it

olblak avatar Dec 08 '22 20:12 olblak