Handy configuration

[sparshev]

Description

Current flatten configuration is not so clear:

• Requires to put sublevels into quotes:

  if( CFG.'some.key.sublevel' ) {
    echo 'Sublevel value exising'
  }

• Disallow to store Maps as usual:

  assert 'sublevel' not in CFG.'some.key' // some.key is not defined

• Looks not great...

So I think there could be a way to handle this bad things and make configuration more durable.

Map with default

def getMapDefault() {
  [:].withDefault {
    getMapWithDefault()
  }
}

config = getMapDefault()

// Now it's possible to get infinity level of the submaps
config.some.key.sublevel = 3
println "${config}" // [some:[key:[sublevel:3]]]

But it has at least one issue:

• Any undefined value will be equal [:] - and it's true for any Map stored in the config. So there is no more null values by default.

MPLConfig object

Specially prepared class that stores the pipeline configuration and handling improper gets - it should determine how much levels we need and only for the last one return null by default... Probably could be handled by groovy, but who know...

[blazmanx34]

Why do you use flatten helper method in MPLModule.groovy file ?
Helper.runModule(module_src, module_path, [CFG: Helper.flatten(cfg)])

Why not provide the parameter cfg directly like this ?
Helper.runModule(module_src, module_path, [CFG: cfg])

By this way, I think config parameters will be accessible like this in module scripts :
CFG.some.key.sublevel

Sorry for my question, I don't understand the usefulness of flattened the config map.

[sparshev]

Hi @blazmanx34,

So there is a number of reasons to do that. What I can remember right now is:

• The main one: ability to use configuration without exceptions:
  For example:CFG.'asd.asd' ?: 'default'
  Otherwise CFG.asd.asd will just generate exception - and exceptions is really hard to manage
• Simplicity of the solution
• A number of not so obvious reasons, like conflicts between the keys: CFG.'asd.asd' != CFG.asd.asd and such things.

So this ticket is exactly about the issue you see - we can't find a better solution to use all the benefits of groovy maps, but here we are.

[blazmanx34]

Thanks for your answer @sparshev

I confess I do not understand why is so important for you to use configuration without exceptions.
The flatten process allow maybe to do that but we lose the "power" of method provided by the Groovy Map class.
There is also solutions with Groovy Map class, to retrieve the value of key map or submap without exceptions (and to specify a default value) :

// Follow this link to execution this script by clicking on 'Edit in console'
// http://groovyconsole.appspot.com/script/5191130064879616
def CFG = [
  name: "myname",
  options: [
    version: "2",
    location: "/var"
  ]
]

println "CFG.name : " + CFG.name
println "CFG.options: " + CFG.options
println "CFG.options.version: " + CFG.options.version
println "CFG.nonExistingOption: " + CFG.nonExistingOption
println "CFG.nonExistingOption?.bar : " + CFG.nonExistingOption?.bar
 
println "Bar default value 2 = " + CFG.get('nonExistingOption', [:]).get('bar', 'defaultbarvalue')

I admit the syntax is a little more complex but it allow to keep the original structure of config map object.

The most important reason for me (I feel that is your third reason) is : prevent conflict between the global config and the module config.
I could suggest to transform moduleConfig method of MPLManager class

public Map moduleConfig(String name) {
  def globalConfig = [global : config.subMap(config.keySet()-'modules')]
  config.modules ? Helper.mergeMaps(globalConfig, (config.modules[name] ?: [:])) : [:]
}

or if using the term 'global' as a reserved word bothers you, you can imagine provides 2 parameters to script modules : CFG (the module params) and GLOBAL (the pipeline config without modules config).

What do you think of my proposals @sparshev ?

[sparshev]

Hi @blazmanx34

Yeah, when I developed this way to access the configuration I also thought about the others like using "?." everywhere, but I don't like that we can't properly find where the vars is used in the modules by usual grep (means save the 'key.key' path separated by dot) - so I think right now it's more the interface issue, neither than an usability issue.
The configuration interface is important, because it allows to reuse modules more or less in the same way, but if there is a number of ways to navigate the config keys - we're getting a different implementations of modules with different styles of using config maps - and I'm pretty sure this will cause a huge compatibility issues in the future.

Another issue - we need to support the old interface (backward-compatibility) - so right now we have no simple way to change this configuration interface as you suggested.

I think the only way right now - is to prepare a special config object based on groovy map, that will provide 2 interfaces: deprecated one CFG.'asd.asd' and new one CFG.asd.asd. If option is not found - result should be null. It's actually not so hard, but needs testing over the already existing community modules.

[sparshev]

So the thing about CFG interface overall - I think it's readability. I dan't want to make it hard to read or understand - modules should save simplicity to be read by almost anyone who even don't know Groovy. Because I think complexity is one of the main issues with the shared libraries in Jenkins Pipeline Engine.

[blazmanx34]

You did not consider Readability and backward-compatibility in your first message.
Personnaly I think that CFG.'asd.asd' is not really readable and is confusing for a developer (even if he don't know Groovy).
We define a config map in the pipeline and this config map is converted to an other map with a different structure in the module script. If this is the same person who develop pipeline and module script. it will be confused.

[sparshev]

You can think about them as "by default" requirements) Noone want to get broken modules with another MPL upgrade and noone want to read bad-looking configs that looks like someone put a cat on the keyboard :)
Anyway, it is what it is - we're looking for a better way to provide the configuration, but still no luck here. There is nothing bad - you can fork the MPL for your own company and try to live with your changes (or use overrides in your nested lib). And I hope soon we will see the results where it goes.

[blazmanx34]

In any case, thanks you for your answers.
Despite the fact that the config management does not please me too much, your library looks great.

I tried to search a solution (extends Groovy NullObject to change the behavior of getProperty method) but in vain...

The only solution I have found is this class which should allow to use config map or the flatten config map

class MPLConfig implements GroovyInterceptable
{
    def config = [:]

    def MPLConfig(Map config = [:]) {
        this.config = config
    }

    def getProperty(String key) {
        String[] keylist = key.tokenize(".")
        def value = config
        keylist.each {
            if (value != null) {
                value = value?.it ?: null
            }
        }
        return value
    }
    
    void setProperty(String key, val) {
        config[key] = val
    }

    def invokeMethod(String name, args) {
        def configMetaClass = config.getMetaClass()
        def configMetaMethod = configMetaClass.getMetaMethod(name, args)
        def result = configMetaMethod.invoke(config, args)
        return result
    }
}

[sparshev]

Hi @blazmanx34,

Thank you for the source code, but I think it's quite not ready. But yeah - overall it's something like that but with much more complex logic for processing something like CFG.asd.asd = 'dsd'...
So we need to think and test a number of ways to make sure the CFG object will work.

I think I can test the config interface like this (with some improvements) - but not soon, unfortunately too busy with the current projects. So if anyone could help - that will be really awesome :)

[sparshev]

I think there is another way to solve this issue and not break the backward-compatibility with minimal code changes: during flatten operation we can leave the original levels of configuration as is and just use links to the values from this maps. Probably this could work like that:

CFG = [
  first1_level: [
    second1_level: [
      third1_value: 5,
      third2_value: "something1",
    ],
    second2_value: 312,
  ],
  first2_level: [
    second3_value: "something2",
  ],
]

flatten(CFG)

groovy.json.JsonOutput.prettyPrint(groovy.json.JsonOutput.toJson(CFG))

And result is:

{
    "first1_level": {
        "second1_level": {
            "third1_value": 5,
            "third2_value": "something1"
        },
        "second2_value": 312
    },
    "first1_level.second1_level": {
        "third1_value": 5,
        "third2_value": "something1"
    },
    "first1_level.second1_level.third1_value": 5,
    "first1_level.second1_level.third2_value": "something1",
    "first1_level.second2_value": 312,
    "first2_level": {
        "second3_value": "something2"
    },
    "first2_level.second3_value": "something2"
}

So we achieving both simple way to call the required values without exceptions and a way to access the maps if we need to by a cost of slightly increased memory usage and multiple keys in the first level of the CFG map.

What do you think guys? Could this work for you?

[sparshev]

Or probably it will be better to still use Config object and provide one-level access (like CFG.'first1_level.second1_level.third1_value') as the main interface.

This interface will promise:

• No exceptions and null value if the key is not found
• Original config map will be properly changed (better than previously suggested flatten modification with duplicated keys with complicated links logic)
• Will allow to access maps directly (like CFG.'first1_level.second1_level') - but when map object is received, it acts like regular map (could cause exceptions and stuff).

So both parties will get what they need - access to the maps and save the old access interface as the main one.

[blazmanx34]

Hi @sparshev ,
to answer to your first proposition : If backward-compatibility is essential for you, I think your proposition could be an acceptable agreement. This solution seems difficult to explain but it meets my needs and your needs.

Map mergeMaps(Map base, Map overlay) {
    if( ! (base in Map) ) base = [:]
    if( ! (overlay in Map) ) return overlay
    overlay.each { key, val ->
      base[key] = base[key] in Map ? mergeMaps(base[key], val) : val
    }
    base
}

Map flatten(Map data, String separator = '.') {
	mergeMaps(data, data.collectEntries { k, v ->
      v instanceof Map ? flatten(v, separator).collectEntries { q, r -> [(k + separator + q): r] } : [(k):v]
    })
}

CFG = [
  first1_level: [
    second1_level: [
      third1_value: 5,
      third2_value: "something1",
	  third3_value: ["el1", "el2", "el3"],
    ],
    second2_value: 312,
  ],
  first2_level: [
    second3_value: "something2",
  ],
]

flatten(CFG)

println groovy.json.JsonOutput.prettyPrint(groovy.json.JsonOutput.toJson(CFG))

The result is

{
    "first1_level": {
        "second1_level": {
            "third1_value": 5,
            "third2_value": "something1",
            "third3_value": [
                "el1",
                "el2",
                "el3"
            ]
        },
        "second2_value": 312,
        "second1_level.third1_value": 5,
        "second1_level.third2_value": "something1",
        "second1_level.third3_value": [
            "el1",
            "el2",
            "el3"
        ]
    },
    "first2_level": {
        "second3_value": "something2"
    },
    "first1_level.second1_level": {
        "third1_value": 5,
        "third2_value": "something1",
        "third3_value": [
            "el1",
            "el2",
            "el3"
        ]
    },
    "first1_level.second2_value": 312,
    "first1_level.second1_level.third1_value": 5,
    "first1_level.second1_level.third2_value": "something1",
    "first1_level.second1_level.third3_value": [
        "el1",
        "el2",
        "el3"
    ],
    "first2_level.second3_value": "something2"
}

However, I think it would be essential to provide 2 parameters to script modules : CFG (the module params as it is provided today for backward-compatibility) and GLOBAL (the pipeline config without modules config), in order to prevent conflict between the global config and the module config.

[blazmanx34]

About your 2nd proposition, I admit I do not imagine what you think.
Do you propose to use a config object like this ?

class MPLConfig
{
    def config = [:]

    def MPLConfig(Map config = [:]) {
        this.config = config
    }

    def getProperty(String key) {
        String[] keylist = key.tokenize(".")
        def value = config
        keylist.each {
            if (value != null) {
                value = value?.it ?: null
            }
        }
        return value
    }
    
    void setProperty(String key, val) {
        config[key] = val
    }
	
    def getConfigMap() {
        return config
    }
}

[sparshev]

Hi @blazmanx34 ,
Yeah like this one - but with interface to use and manage nested hashmaps via flattened keys. Means flattened keys will be virtual and both get/set operations will actually get/set the original config map.

So it's very like your previous offer - I just removed some requirements I had in my head (like the access without exception to not existing key in manner like CFG.first1_level.second1_level_notexist - this still will generate an exception, but CFG.'first1_level.second1_level_notexist' will return just null or if it's set - will create the required keys in the map).

[sparshev]

Preparing the implementation right now - yeah, even with this quite simple interface - there is alot of corner points... Hopefully unit tests will help to make sure I did not miss anything.

[blazmanx34]

Thanks, I can now more understand your proposition.
It looks great for me.
However, don't you want to prevent conflict between the global config and the module config ?

For exemple, if we have a pipeline config map like that :
(Sorry I don' have real case)

[
    agent_label: '',
    param: 'value 1',
    modules: [
      Checkout: [:],
      Build: [
           param: 'value 2',
      ],
      Deploy: [:],
      Test: [:]
    ]
]

The Build module config will be :

[
    agent_label: '',
    param: 'value 2',
]

and we have losen the global param value.

Maybe, it's not a problem... Is it stupid for a module to use a pipeline config parameter ?

[blazmanx34]

Is it possible to implement GroovyInterceptable Interface in your MPLConfig class, and add the following method :

    def invokeMethod(String name, args) {
        def configMetaClass = config.getMetaClass()
        def configMetaMethod = configMetaClass.getMetaMethod(name, args)
        def result = configMetaMethod.invoke(config, args)
        return result
    }

By this way, we could use the Map methods too (it could be useful).

For example :

CFG = MPLConfig.create([...])
CFG.get('firstx_level','default')

[sparshev]

Hi @blazmanx34

However, don't you want to prevent conflict between the global config and the module config ?

Probably you missing the point of modules in the pipeline config. It's here just to simplify modifications for the existing pipeline and simplify the pipeline configuration overall. There is no such thing as "modules" configuration - there is just overrides for the global config for a specific module (actually right now for our pipelines it's working only for stages, because inside the stage module we usually passing the the whole CFG to the children module).

Probably this modules interface will gone some day and we will deprecate it as not clear, but the thing is - it's not a modules configuration, it's overrides for the specific modules of some general pipeline.

Overall splitting the configuration to global and module looks not good - that will require to write specific module configuration instead of providing just one CFG config entry point for module.

CFG = MPLConfig.create([...])
CFG.get('firstx_level','default')

Don't really like how this is looks like - it just creates another way for providing default through the function without elvis operator. But elvis is much more powerful than this get interface, for example:

CFG.'required.config.value' ?: error('Required variable is not set')

So I don't think we will provide such alternative if it gives nothing but a way to write the same get config in a different way.

[blazmanx34]

OK I think I don't understand the global library philosophy.... :-(

If I understand, the modules section in the pipeline config map is used to enable or disable a module and we can specify a parameter for a particular module to override the global config.

I'm right ?

[sparshev]

Yeah, right)

[sparshev]

In theory that was a good approach - but practice as usual corrected the plans: Security plugin denies to access get/setProperty, so for now we're doomed: https://github.com/jenkinsci/script-security-plugin/blob/script-security-1.60/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/blacklist#L7

Trying to find workarounds - this approach is too good, but right now see no good ways...

[sparshev]

I got some progress with using implements Map, but need to work on the unit tests, they are failing for some reason...

[sparshev]

Great news - I found a way to workaround the Security Plugin restriction by using Map as an interface, so the PR was modified and ready for review.