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.

Sign up for GitHub

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

Jump to bottom

Add a merge strategy extension point for the YAML config #1218

Merged
merged 17 commits into from
Sep 14, 2021
Merged

Add a merge strategy extension point for the YAML config #1218

merged 17 commits into from
Sep 14, 2021

Conversation

LinuxSuRen
Copy link
Member

@LinuxSuRen LinuxSuRen commented Dec 6, 2019 •
edited by jetersen
Loading

Your checklist for this pull request

fix #1194

🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

jetersen
jetersen reviewed Dec 6, 2019
View reviewed changes
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of CasCGlobalConfig, should it not be SelfConfigurator?

@timja
Copy link
Member

timja commented Dec 6, 2019

Looks great!

@LinuxSuRen
Copy link
Member Author

Any more feedbacks are appreciated.

timja
timja previously approved these changes Dec 10, 2019
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for each merge strategy please? I think we’re good to go after that

@jetersen
Copy link
Member

jetersen commented Dec 10, 2019 •
edited
Loading

without test case I have no idea how to change merge strategy 🤔
Instead of system property I think it should be apart of the configuration context ie. self configurator.
This way it is declarative.

@timja timja dismissed their stale review December 10, 2019 07:56

didn't mean to approve, was on phone

@LinuxSuRen
Copy link
Member Author

Instead of system property I think it should be apart of the configuration context ie. self configurator.
This way it is declarative.

@Casz Good advice. I do my best to try with this solution.

@timja timja changed the title Add a merge stragtegy extension point for the YAML config Add a merge strategy extension point for the YAML config Dec 10, 2019
@timja timja added the feature A PR that adds a feature - used by Release Drafter label Dec 10, 2019
@LinuxSuRen
Copy link
Member Author

The merge strategy should be set before starting to parse the YAML config file. I didn't find out how can I set ConfigurationContext at that point. @Casz Please give me some tips if you know it. Thanks.

@jetersen
Copy link
Member

jetersen commented Dec 10, 2019 •
edited
Loading

@LinuxSuRen in the ConfigurationContext class you would add the merge strategy field

configuration-as-code-plugin/plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationContext.java

Line 15 in 80e556e

public class ConfigurationContext implements ConfiguratorRegistry {

This will be the first component configured thanks to:

configuration-as-code-plugin/plugin/src/main/java/io/jenkins/plugins/casc/impl/configurators/SelfConfigurator.java

Lines 18 to 20 in 80e556e

@Extension(ordinal = Double.MAX_VALUE)
@Restricted(NoExternalUse.class)
public class SelfConfigurator extends BaseConfigurator<ConfigurationContext> implements RootElementConfigurator<ConfigurationContext> { 

configuration-as-code:
  mergeStrategy: order
  deprecated: warn

@LinuxSuRen
Copy link
Member Author

@Casz If there're two config files which all contain mergeStrategy, it'll cause the conflicts.

@jetersen
Copy link
Member

@LinuxSuRen then they are also asking for trouble, let's assume that people won't conflict on it.

jetersen
jetersen reviewed Dec 11, 2019
View reviewed changes
plugin/src/main/java/io/jenkins/plugins/casc/yaml/DefaultMergeStrategy.java Outdated
import java.util.Iterator;

@Extension
public class DefaultMergeStrategy implements MergeStrategy {
Copy link
Member

@jetersen jetersen Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Default is a terrible name it does not describe the merge behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StrategicMerge? It’s the name kustomize used for ‘smart’ merge

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just throws an exception when there's conflict exists.
What about IrreconcilableMergeStrategy?

@LinuxSuRen
Copy link
Member Author

Another problem is that it looks like I can get the right value of mergeStrategy before all the config files were loaded if I put mergeStrategy into ConfigurationContext.

@LinuxSuRen LinuxSuRen requested a review from a team as a code owner February 12, 2020 09:27
@LinuxSuRen
Copy link
Member Author

I messed up the git commit history before. I wish we can push it forward.

@timja
Copy link
Member

timja commented Feb 12, 2020

@LinuxSuRen are you able to implement this?
#1218 (comment)

@LinuxSuRen
Copy link
Member Author

@LinuxSuRen are you able to implement this?
#1218 (comment)

Yes, but I think the merge strategy should be loaded before the YAML file is actually parsed.

@jetersen
Copy link
Member

@LinuxSuRen would be great to land this, do you mind fixing the conflicts.
We recently had a few changes around the YamlUtils.

@LinuxSuRen
Copy link
Member Author

@jetersen Sorry that I missing your message here. I'll fix them in recent days.

jetersen
jetersen reviewed Jul 22, 2020
View reviewed changes
plugin/src/main/java/io/jenkins/plugins/casc/yaml/YamlUtils.java Outdated
/**
* Load configuration-as-code model from a set of Yaml sources, merging documents
*/
public static Mapping loadFrom(List<YamlSource> sources) throws ConfiguratorException {
public static Mapping loadFrom(List<YamlSource> sources, MergeStrategy mergeStrategy) throws ConfiguratorException {
Copy link
Member

@jetersen jetersen Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessary to pass merge strategy here. Properly simpler to call MergeStrategyFactory.getMergeStrategy() inside the merge method

Copy link
Member

@jetersen jetersen Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think it would be best to move the call to ConfigurationContext constructor and store the merge strategy in the context

@LinuxSuRen LinuxSuRen mentioned this pull request Jul 24, 2020
Open
2 tasks
@oleg-nenashev
Copy link
Member

Are we still interested in this pull request?

@LinuxSuRen
Copy link
Member Author

can you update docs please

See it from here. I also put a link on the README file.

@jetersen jetersen mentioned this pull request Aug 24, 2021
Closed
3 tasks
timja
timja reviewed Aug 24, 2021
View reviewed changes
docs/features/mergeStrategy.md Outdated

## Support strategies

* [IrreconcilableMergeStrategy]((../../plugin/src/main/java/io/jenkins/plugins/casc/yaml/IrreconcilableMergeStrategy.java)) (default)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just match the class name to the subtracting mergestrategy?

so the class should be called ErrorOnConflictMergeStrategy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

timja
timja reviewed Aug 24, 2021
View reviewed changes
docs/features/mergeStrategy.md Outdated
It's possible to load multiple config files. CasC can load YAML files from a directory.
And it's convenient to maintain if we split different parts of Jenkins into multiple files.

## Support strategies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Support strategies
## Supported strategies

timja
timja reviewed Aug 24, 2021
View reviewed changes
docs/features/mergeStrategy.md Outdated

* [IrreconcilableMergeStrategy]((../../plugin/src/main/java/io/jenkins/plugins/casc/yaml/IrreconcilableMergeStrategy.java)) (default)
* The strategy name is `errorOnConflict`.
* Throw an exception if there's a conflict exist in multiple YAML files.
Copy link
Member

@timja timja Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Throw an exception if there's a conflict exist in multiple YAML files.
* Throws an exception if there's a conflict in multiple YAML files.

timja
timja reviewed Aug 24, 2021
View reviewed changes
docs/features/mergeStrategy.md Outdated
Comment on lines 17 to 18
You can provide two YAML config files. They can be system and user config files. Then allow users to change the users' part.
So, the users' config file can override the system one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe?

Suggested change
You can provide two YAML config files. They can be system and user config files. Then allow users to change the users' part.
So, the users' config file can override the system one.
You can provide two YAML config files. They can be system and user config files. Then allow users to change their file and override the system configuration.

@LinuxSuRen LinuxSuRen mentioned this pull request Aug 24, 2021
Merged
@timja timja dismissed their stale review August 25, 2021 15:10

addressed

@timja
Copy link
Member

timja commented Aug 25, 2021

Code looks good from reading it, when I get a chance I'd like to try this out myself locally, and then merge if all is fine.

(That doesn't stop another maintainer from merging this though)

@JohnNiang
Copy link
Member

How is it going recently? I'm looking forward to this PR to get merged. : P

Hi all reviewers, I really hope to see it merged quickly.

@timja
Copy link
Member

timja commented Aug 30, 2021

How is it going recently? I'm looking forward to this PR to get merged. : P

Hi all reviewers, I really hope to see it merged quickly.

You could try it out and provide feedback. I’m away currently, back tomorrow, but I’ve got a backlog to work through, I hope to get to it soon

@JohnNiang
Copy link
Member

JohnNiang commented Sep 8, 2021 •
edited
Loading

hi @timja , I've tested against this feature. Everything is ok to me.

@timja
Copy link
Member

timja commented Sep 9, 2021 •
edited
Loading

I tested 4 scenarios, I think the merging needs to be a bit smarter, I was especially surprised that systemMessage overriding doesn't work

systemMessage override ❌ 
# 1.yaml
jenkins:
  systemMessage: "hello1"
# 2.yaml
jenkins:
  systemMessage: "hello2"

expected:

jenkins:
  systemMessage: "hello2"

actual

jenkins:
  systemMessage: "hello1"
simple globalNodeProperties override ✔️ 
# 1.yaml
jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE1
            value: foo
# 2.yaml
jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE2
            value: bar

expected:

jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE1
            value: foo
          - key: VARIABLE2
            value: bar

actual

jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE1
            value: foo
          - key: VARIABLE2
            value: bar
globalNodeProperties override with another sequence object ❌ 
# 1.yaml
jenkins:
  systemMessage: normal
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE1
            value: foo
  agentProtocols:
    - "jnlp1"

# 2.yaml
jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE2
            value: bar
  agentProtocols:
    - "jnlp2"

expected:

jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE1
            value: foo
          - key: VARIABLE2
            value: bar

actual

jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE2
            value: bar
build discarder override ✔️ 
# 1.yaml
unclassified:
  buildDiscarders:
    configuredBuildDiscarders:
      - "jobBuildDiscarder"

# 2.yaml
unclassified:
  buildDiscarders:
    configuredBuildDiscarders:
      - simpleBuildDiscarder:
          discarder:
            logRotator:
              daysToKeepStr: "7"
              numToKeepStr: "10"

expected:

unclassified:
  buildDiscarders:
    configuredBuildDiscarders:
    - "jobBuildDiscarder"
    - simpleBuildDiscarder:
        discarder:
          logRotator:
            daysToKeepStr: "7"
            numToKeepStr: "10"

actual

unclassified:
  buildDiscarders:
    configuredBuildDiscarders:
    - "jobBuildDiscarder"
    - simpleBuildDiscarder:
        discarder:
          logRotator:
            daysToKeepStr: "7"
            numToKeepStr: "10"

@timja
Copy link
Member

timja commented Sep 13, 2021

Retested all,

globalNodeProperties override with another sequence object is still failing.

I assume agentProtocols change is getting silently rejected which is fine, but the lists should be being merged so I get:

jenkins:
  globalNodeProperties:
    - envVars:
        env:
          - key: VARIABLE1
            value: foo
          - key: VARIABLE2
            value: bar

but I'm just getting:

globalNodeProperties:
  - envVars:
      env:
      - key: "VARIABLE2"
        value: "bar"

@LinuxSuRen
Copy link
Member Author

globalNodeProperties override with another sequence object is still failing.

I found it. I'm still working on it. But I think it might be more complicated. Let's assume that there are two kubernetes cloud configuration items. It's very difficult to merge them into one. The biggest challenge is to identify which items belong to the same one.

I'm not sure if I explain it well. But I'm looking forward more feedback on this.

@LinuxSuRen
Copy link
Member Author

hi @timja , I didn't figure out a good solution for this. Can we improve it later? IMO, this is an improvement point instead of a bug. I will use this feature in the project ks-devops if you agree and merge it.

@timja timja merged commit 9cc1a86 into jenkinsci:master Sep 14, 2021
@timja
Copy link
Member

timja commented Sep 14, 2021

hi @timja , I didn't figure out a good solution for this. Can we improve it later? IMO, this is an improvement point instead of a bug. I will use this feature in the project ks-devops if you agree and merge it.

sure thanks for your effort and patience on this.

@LinuxSuRen LinuxSuRen deleted the merge-strategy branch September 14, 2021 10:37
@LinuxSuRen
Copy link
Member Author

Thanks to all contributors help me to review this PR. I'm looking forward to the release of this plugin. I'm not sure if @timja has the release permission. Please help me with it if you think it's ok.

@timja
Copy link
Member

timja commented Sep 14, 2021

it's in progress

This was referenced Sep 15, 2021
Merged
Merged
@JohnNiang JohnNiang mentioned this pull request Dec 23, 2021
Closed
@codemuncha
Copy link

Hello,

I have a suggestion to improve the documentation for the merge strategy. As a Jenkins administrator, I find the current information less helpful, especially for those who are not Java developers. It would be great if we could make the documentation clearer and more user-friendly, catering to the needs of all administrators, regardless of their programming background.

Thank you for considering this improvement.

Best regards,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jetersen jetersen jetersen left review comments

@JohnNiang JohnNiang JohnNiang left review comments

@timja timja timja left review comments

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

Assignees
No one assigned
Labels
feature A PR that adds a feature - used by Release Drafter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add YAML merge strategy to meet different use cases
8 participants
@LinuxSuRen @timja @jetersen @oleg-nenashev @max-allan-surevine @ilakhtenkov @JohnNiang @codemuncha