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

Merge configurations by unique IDs #54

Open
gentlementlegen opened this issue Jun 12, 2024 · 9 comments
Open

Merge configurations by unique IDs #54

gentlementlegen opened this issue Jun 12, 2024 · 9 comments

Comments

@gentlementlegen
Copy link
Member

gentlementlegen commented Jun 12, 2024

We should look into a more robust configuration merge.

Quote from discussion:

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.

Originally posted by @0x4007 in #47 (comment)

@gentlementlegen
Copy link
Member Author

Now that we are creating a configuration editor, I feel like this is not really needed and could introduce a lot of complexity and errors while we could easily handle configurations through the UI what do you think @0x4007

@rndquu
Copy link
Member

rndquu commented Nov 29, 2024

@gentlementlegen I've actually thought this task is for merging organization and repository configs by unique plugin ids/keys, no?

@gentlementlegen
Copy link
Member Author

This would be doable but because we now have multiple keys and reusable anchors in our configuration I am not sure how this can be achieved.

@rndquu
Copy link
Member

rndquu commented Dec 2, 2024

This would be doable but because we now have multiple keys and reusable anchors in our configuration I am not sure how this can be achieved.

You mean that uses is an array?

@gentlementlegen
Copy link
Member Author

Yes both plugins and uses are arrays. Try the tool for installing the configuration through the web UI and you'll see how anchors are also used to populate the configuration, which I don't think is mergeable.

@0x4007
Copy link
Member

0x4007 commented Dec 2, 2024

There are two scenarios for matching plugin ids:

  1. org + repo config merged
  2. repo + repo config (aka two instances of matching plugin id)

In the first scenario, we should clobber the org plugin setting with the repo plugin setting. This is consistent with our intent to allow for local repository level overrides against the organization wide config.

In the second scenario, this is most likely to be an error. I think that this should throw an error when pushing these changes. We should also throw an error if this is detected in our UI. We should not support this configuration in the UI or with the kernel.

@henalolp
Copy link

henalolp commented Dec 3, 2024

/help

@rndquu
Copy link
Member

rndquu commented Dec 3, 2024

This would be doable but because we now have multiple keys and reusable anchors in our configuration I am not sure how this can be achieved.

You mean that uses is an array?

@gentlementlegen What's the use case for uses to be an array?

@gentlementlegen
Copy link
Member Author

@rndquu it should be an array when we want plug-ins to run sequentially and forward the outputs to each other. When placed at top level, execution has not order. When chained in the uses section, they run one after another and send the payloads one to another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants