Skip to content
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

Suggestion for configuration #21

Open
digulla opened this issue Mar 13, 2019 · 10 comments
Open

Suggestion for configuration #21

digulla opened this issue Mar 13, 2019 · 10 comments
Labels
enhancement New feature or request

Comments

@digulla
Copy link

digulla commented Mar 13, 2019

This is another approach to enhance the config (similar to #10).

I would suggest a "default config" object which contains all possible config keys and defaults.

Modules must have a method to add to this default config. Adding the same "path" twice is an error but it should be possible to check for an existing path (so two modules can cooperate and still be optional).

This way, modules don't need code like

CFG.'maven.tool_version' ?: 'Maven 3'

because 'Maven 3' will be the default value. Reading unknown keys should throw an exception (-> typo in library)

The step which parses the project's config options from the should throw exceptions for unknown keys/paths to catch typos.

After merging the project and default config, the result should be logged in Jenkins to allow to find mistakes.

I'm also not fond of the CFG.'xxx.yyy' syntax. With the default config as path supplier, it should be possible to implement a nested config using nested Maps which would then allow to pass inner maps around so the code would become independent of the path.

In my builds, I'm calling Maven several times (build with unit tests, site, deploy without tests). Using nested configs, I can create a shared Maven config (default JDK & Maven tool) but I can also tweak the options passed to each individual Maven invocation like additional profiles to activate or system properties. As an example, I can:

        config {
            build {
                addAfter 'install', 'foo'
            }
        }

to get mvn [...] clean install foo sonar:sonar as command line or I can

        config {
            maven {
                property('foo', params['foo'])
            }
        }

to add -Dfoo=[...] to every Maven invocation.

My current code depends on a hand-written config builder class for each nested config object. That way, I can support things like addAfter(). I'm looking into ways to change this so modules can easily define their default config.

@sparshev
Copy link
Collaborator

Hello @digulla,

Happy to see you thinking about the improvements for MPL configuration) Unfortunately it's quite hard to understand the idea without a code, but seems like you proposing for some huge interface changes.
I'm wondering to see the fork / nested shared lib / PR with some simple examples to compare the approach with already provided one, how this will affect the interfaces and to imagine the future of proposed configuration schema.
So we can discuss the changes and choose the right way of the further improvements.

Thank you

@sparshev sparshev added the enhancement New feature or request label Mar 14, 2019
@digulla
Copy link
Author

digulla commented Mar 14, 2019

I've created a gist with unit tests: https://gist.github.com/digulla/15a55d8a5cefb2bda1e2da1fb6dae4f7

I'd like to first discuss this code and then start with a fork.

@sparshev
Copy link
Collaborator

Thank you @digulla , we will check it shortly.

@sparshev
Copy link
Collaborator

sparshev commented Mar 20, 2019

Hi @digulla,

I checked the source code and from the tests I see, that the configuration becomes quite bulky. Also it's hard to predict where this will go. I think it just will move a part of the module logic to the configs, that seems not good...
You saying, that there will be no need of CFG.'maven.tool_version' ?: 'Maven 3' - but I think it will be just moved somewhere else...
Anyway it's still hard to say for sure using this code, because probably I don't understand it correctly. So it will be good to see the fork with your idea implementation to compare with the existing solution.

Thank you

@digulla
Copy link
Author

digulla commented Mar 22, 2019

My goals are:

  • I want to be able to print the configuration before it's being used to make it possible to debug failed builds
  • Prevent typos and wrong types
  • Make it easy to spot name collisions
  • Print useful error messages when something is wrong with the config
  • Avoid typical beginner errors like {name: 'value'}. A lot of people using this won't be Groovy experts.
  • Support for subtrees (i.e. the same config layout with different paths)

You saying, that there will be no need of CFG.'maven.tool_version' ?: 'Maven 3' - but I think it will be just moved somewhere else...

It has to be somewhere. With your design, all plugins accessing this config option must use the same pattern to read it. Specifically, the default value must be the same everywhere or weird things will happen ("hey, why is it doing this here and that there????").

In my design, the default config does it for everyone, so everyone can simply use CFG.maven.tool_version and they will get a non-null String. Defaults don't have to be kept in sync in several places.

I think we should talk about my design goals. I think they are worth the effort because they help to catch a lot of bugs.

@sparshev
Copy link
Collaborator

Yeah, agree. We just need to compare the differences of approaches, the interface changes, required efforts to implement and backward compatibility to make some other decisions.
Right now I think that probably it's possible to give a way for the end users to test this new way. I think it will be much easier to play with the fork and on practice see the benefits and maybe the negative points.

@digulla
Copy link
Author

digulla commented Mar 25, 2019

I ran into an interesting question while trying to implement my idea. Where to put the default config?
I see threse options:

  • It could be put into the modules and the set up code could use regexp to parse it out.
  • We could move all the code in the modules into a closure which would be part of the default config.
  • We could turn the modules into Groovy types with a run() and getConfig() method.
  • We could put the config into a new file next to the module with the same base name + CFG.groovy.

I don't like option 1 very much. Only if we're very desperate.

With option 2, I expect name collisions when we merge the override configs (configs in the projects).

With option 3, a lot of code would have to be changed. Also, CPS might cause problems. Lastly, people might try to put state into those classes and we would have to decide how to handle that.

Option 4 sounds doable but I would like to have the config and the code in a single file. Otherwise, we might set a precedent and end up with tons of file fragments.

That aside, you currently have some logic to prevent endless loops when a module calls it's parent. With option 2 and 3, the parent code could be passed to the override module as closure.

@sparshev
Copy link
Collaborator

  • First - regexp word looks not good...
  • Second - modules was prepared as opposite of vars - it's just a plain file and all the control logic was planned to be outside. Inside - just some simple structure like plain pipeline file.
  • Third - seems will increase complexity of the modules and will ruin the backward compatibility as the second one.
  • Fourth - can't say for sure, since I do like both python and c++ ways... But yeah, I think it will be better to leave just one file per module, otherwise the structure of MPL will be shattered...

Probably it will be suitable to create vars/MPLModuleDefaults.groovy with logic to set module defaults and prepend each module with such call with specifying defaults map... It will move defaults to the module top that is pretty close to the rest of the module logic, will slightly increase complexity, but will be backward-compatible with the old way...

@digulla
Copy link
Author

digulla commented Mar 27, 2019

I would really like to collect all config options before starting to build.

Maybe a solution would be to use a different approach for the pipeline: Instead of using explicit steps, we could create the steps dynamically (see https://stackoverflow.com/questions/42837066/can-i-create-dynamically-stages-in-a-jenkins-pipeline).

That way, we could collect all the modules which are part of the build beforehand. With a flag, we could disable executing the "build code" module. Then we run all the modules to collect all config options.

With that, we can merge the overrides for the current build, log the result, enable execution again, disable the code in MPLModuleDefaults (otherwise, we'd get duplicate key errors) and do the actual build.

The other solution would be to run all modules that we can find but that would run Checkout.groovy, DefaultCheckout.groovy and GitCheckout.groovy. I'm uneasy about this.

Or is there a way to discover the stages in a Jenkinsfile while running it? Isn't Jenkins showing all stages before it starts executing the first one?

@sparshev
Copy link
Collaborator

Hi @digulla

You can use any pipelines you want - default MPLPipeline is just to show a simple example pipeline.
For my project I'm using the same structure, but with the custom logic and agents per stage (because we using input for redeploy and just to save some money).

Sorry, but it's still overcomplicated to just understand what you saying without some logic to play with... I'm usually just trying one way, that if something is not working perfect enough - another and so on.

There for sure some ways (at least you can use vars steps and going deep into the jenkins objects to locate what you need) - but be careful, noone likes too much complicated logic...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants