feat: read org config

[gentlementlegen]

Resolves #46

[gentlementlegen]

@0x4007 @whilefoo This aims to solve the fact that the new bot doesn't run on repos that do not contain a configuration, because the kernel doesn't check for a repo wide configuration. Do we prefer overriding that repo configuration is one is found in the repo, or merge both instead?

[whilefoo]

Before we used to merge repo and org config where repo overrode org if two fields were both present. In the new bot it's gonna be a bit more complicated because we have arrays

Repo config:

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: true
        - plugin: something/plugin-2

Org config:

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: false
            test2: false
        - plugin: something/plugin-2
    - uses:
        - plugin: something/plugin-3

Option 1 (we merge plugin chains if they contain same plugins in the same order)

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: true
            test2: false
        - plugin: something/plugin-2
    - uses:
        - plugin: something/plugin-3

Option 2 (we treat all plugin chains as unique)

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: true
        - plugin: something/plugin-2
    - uses:
        - plugin: something/plugin-1
          with:
            test: false
            test2: false
        - plugin: something/plugin-2
    - uses:
        - plugin: something/plugin-3

[gentlementlegen]

To me option 1 looks safer because in option 2 we would potentially have the same plugin fired multiple times, which I do not think is wanted, even with different configurations.

[0x4007]

I think that we should check for collisions in plugin ID and then if it exists, prefer repo config over org.

To me that seems by far the most partner friendly and sensible conclusion.

[gentlementlegen]

I've been playing with this for quite some time, and I think any sort of merge can introduce lots of unwanted side effects, such as plugins being duplicated and running multiple times, or configurations using the wrong with values.

I think the most common use cases to have a different configuration in a repo would be to only run specific plugins there or use specific values for some plugin payload. So I think the repo configuration should take complete precedence over the org, expect if nothing is specified for an event. Example:

Repo config:

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: true
        - plugin: something/plugin-2

Org config:

plugins:
  issues.opened:
    - uses:
        - plugin: something/plugin-4
          with:
            test: true
        - plugin: something/plugin-4
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: false
            test2: false
        - plugin: something/plugin-2
    - uses:
        - plugin: something/plugin-3

Would result in

plugins:
  issues.opened:
    - uses:
        - plugin: something/plugin-4
          with:
            test: true
        - plugin: something/plugin-4
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: true
        - plugin: something/plugin-2

This way, it is easy to override the Org config if needed, but also easy to keep it as it is and build on top of it. Let me know what you think.

[0x4007]

I've been playing with this for quite some time, and I think any sort of merge can introduce lots of unwanted side effects, such as plugins being duplicated and running multiple times, or configurations using the wrong with values.

Please reread my comment. If the plugin has a collision with its ID then take the repo defined config.

Merge conflicts are impossible because it's not using the plugin settings from the org in case of collision.

[gentlementlegen]

@0x4007 What exactly do you call plugin ID? Even though their names are unique it is valid to include the same plugin several time even in the same event, so I am not sure to understand.

[0x4007]

Why would a config have the same plugin invoked back to back? Even if, this should be expressed inside of the plugin config section ("run twice") instead of including the same plugin ID in the config twice.

Can you generate an example config showing what you mean? It's a bit tedious for me to at this moment from my phone.

[gentlementlegen]

@0x4007 It would not be very useful for now but it technically is a valid scenario.

plugins:
  '*':
    - uses:
        - plugin: something/plugin-1
        - plugin: something/plugin-1
        - plugin: something/plugin-1
  issues.opened:
    - uses:
        - plugin: something/plugin-1
        - plugin: something/plugin-1
        - plugin: something/plugin-1
    - uses:
        - plugin: something/plugin-1
        - plugin: something/plugin-1
        - plugin: something/plugin-1

The same way in Github actions where you can call multiple times the same Action inside a workflow.

[0x4007]

Does it solve the problem if we generate virtual keys at runtime to express the plugin and webhook event association?

"issues.created.something/plugin-1"

"*.something/plugin-1"

etc

Then refer to my suggestion

[rndquu]

We can simply treat event names as unique keys and override based on them without complicated merging of plugin settings or chains. That would enough to start. Example:

Org config:

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-1
          with:
            test: false
            test2: false
        - plugin: something/plugin-2
    - uses:
        - plugin: something/plugin-3

Repo config:

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-4

Results in:

plugins:
  issues.closed:
    - uses:
        - plugin: something/plugin-4

[gentlementlegen]

@rndquu Agreed, it seems to be the most straightforward and less error prone solution. Plus, I think we will use org config in 95% of cases, so it should be fine for edge use-cases.

[whilefoo]

We can simply treat event names as unique keys and override based on them without complicated merging of plugin settings or chains.

I don't think overriding event names will be a useful use case but we can start with that and later when we actually use the bot and see what kind of behavior we need, we can come back to this.

[0x4007]

@0x4007 It would not be very useful for now but it technically is a valid scenario.

plugins:
  '*':
    - uses:
        - plugin: something/plugin-1
        - plugin: something/plugin-1
        - plugin: something/plugin-1
  issues.opened:
    - uses:
        - plugin: something/plugin-1
        - plugin: something/plugin-1
        - plugin: something/plugin-1
    - uses:
        - plugin: something/plugin-1
        - plugin: something/plugin-1
        - plugin: something/plugin-1

The same way in Github actions where you can call multiple times the same Action inside a workflow.

We can simply treat event names as unique keys and override based on them without complicated merging of plugin settings or chains.

I don't think overriding event names will be a useful use case but we can start with that and later when we actually use the bot and see what kind of behavior we need, we can come back to this.

With my experience creating GitHub Actions CI scripts there is no situation I recall with duplicate dependencies being called. Sure its technically possible to do, but its not a realistic situation in my experience. I'm more concerned with addressing practical problems vs academic.

[gentlementlegen]

@0x4007 I think the difference is that in GitHub there is no such concept as merging configurations together. At this moment, the logic is to use the events as unique keys, as observed in this test.

[0x4007]

I know there isn't a concept of merging configurations together on GitHub Actions.

This doesn't address the key point I am making. The cause we are discussing is a collision with dependency IDs. And whether there are real-world scenarios where configurations might lead to duplicate dependencies.

From my experience creating GitHub Actions CI scripts, I haven't encountered situations where adding redundant dependencies was necessary. This makes the theoretical merging logic problem irrelevant to our practical implementation.

We should focus on the actual conditions under which our configurations operate, rather than hypothetical scenarios that don't align with real-world use cases.

Basically I'm saying to drop support for using the same dependency more than one, particularly when associated with the same webhook event. See this comment for a suggestion on how to handle that.

[gentlementlegen]

@0x4007 Opened another issue on that matter as I believe it falls out of scope of this feat: #54