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

Use the proper configuration (repo scoped) #68

Open
gentlementlegen opened this issue Dec 25, 2024 · 17 comments
Open

Use the proper configuration (repo scoped) #68

gentlementlegen opened this issue Dec 25, 2024 · 17 comments

Comments

@gentlementlegen
Copy link
Member

          @0x4007 So the configuration is correct and does not require a pull-request as you can see [here](https://github.com/ubiquity/business-development/blob/development/.github/.ubiquity-os.config.yml#L370). However during the run that value was `true` as you can see [here](https://github.com/ubiquity-os-marketplace/daemon-disqualifier/actions/runs/12478009115/job/34824958340#step:3:26). This is probably due to the fact that the run was triggered from another repo where that value is `true`. Not sure how we should handle this scenario.

Originally posted by @gentlementlegen in ubiquity/business-development#103 (comment)

Copy link

Note

The following contributors may be suitable for this task:

gentlementlegen

77% Match ubiquity-os-marketplace/command-start-stop#74

@gentlementlegen gentlementlegen changed the title Use proper configuration Use the proper configuration (repo scoped) Dec 25, 2024
@gentlementlegen
Copy link
Member Author

@0x4007 continuing the discussion here to not pollute the other issue.

I think the proper solution is to only run the disqualifier checks within the repo where the event was triggered (not the whole org). This is because we can have repo scoped configurations, which only apply to that specific repo.

@0x4007
Copy link
Member

0x4007 commented Dec 25, 2024

I disagree. There's many repos without much activity and a few with the most so the former would be very unmaintained by the disqualifier.

Instead the disqualifier should build a mapping of every (merged) config to every repo.

Ideally it should use the identical SDK logic the kernel uses to interpret the configs.

@gentlementlegen
Copy link
Member Author

Then we should simply ensure that the disqualifier runs not based on event but time intervals.

Having the disqualifier being able to read configs would just lead to code duplication and complex workflow logic, which should be avoided. Also if the base org does not have the disqualifer but the repo does have it, it means that either way events would only be triggered from this repo and the config would apply to other repos (which should not happen since the org does not have the plugin enabled).

@0x4007
Copy link
Member

0x4007 commented Dec 26, 2024

I'm not convinced. I think the SDK handles most of the config related logic so duplication is fine because it's imported.

@gentlementlegen
Copy link
Member Author

The SDK does not handle the configuration for plugins, only the kernel knows how to read / decode / validate it. Plugins are only aware of their own configuration which is sent by the kernel.

Consider the following scenario:

  • the organization does not have daemon-disqualifier enabled
  • this repository has daemon-disqualifier enabled

in this case, the daemon-disqualifier will run in the whole org regardless, which I don't think is a desired behavior.

@0x4007
Copy link
Member

0x4007 commented Jan 1, 2025

@whilefoo rfc

I'm not sure what else can be done here

@gentlementlegen
Copy link
Member Author

To me the solutions are either:

  • run the disqualifier only on the repo where the event happened;
  • run the disqualifier on a time based interval to not rely on events happening

The base of the problem is calling all the repositories for an event.

@0x4007
Copy link
Member

0x4007 commented Jan 2, 2025

Another idea is to hand off the event from the kernel to a GitHub Action, and have the Action continue to retry, with an exponential backoff, if there are failures.

In less than six hours it should be able to sort itself out with this strategy.

@gentlementlegen
Copy link
Member Author

Yes but the run itself is triggered from the kernel, which tells the plugin to run. The problem is that this plugin runs on the whole organization using its own configuration which should be specific to its repository only.

@0x4007
Copy link
Member

0x4007 commented Jan 2, 2025

which should be specific to its repository only.

No my idea is that it should run on every repository in the organization. The problem we are trying to solve is related to rate limits and exponential backoffs are a solution to this.

@gentlementlegen
Copy link
Member Author

gentlementlegen commented Jan 2, 2025

Yes but this issue was filled to fix the following scenario:

  • business-development is configured to not disqualify on missing pull-request
  • daemon-disqualifier was triggered from another repository in the ubiquity organization, which was configured to disqualify on missing pull-request
  • daemon-disqualifier evaluated issues in business-development using the other repository configuration, thus disqualifying the user

Last step is incorrect, and would be solved by running daemon-disqualifer only in the repo where the event happened.

@whilefoo
Copy link
Member

whilefoo commented Jan 3, 2025

There's no easy answer to this.
We could implement cron jobs but that would hit rate limits even faster because on every kernel run (for example every minute) it would have to check every config that exists and see if there's a cron plugin that needs to be run.
Fetching all repo configs on every org event is also not a good idea for rate limits but can be a fix for the problem of correct configs.
A temporary fix would be to put the plugin in org config and specify all repos that require a PR.

@gentlementlegen
Copy link
Member Author

I had an idea that maybe would solve our problems, without needing to change anything inside of the kernel nor implementing crons within our system.

To handle the cron part, we could have an Action script like cron.yml which would trigger once every 24h, which only purpose is to trigger a compute.yml run.

The workflow would be the following:

  • daemon-disqualifier would run the same as it is now
  • at the end of the run, we would store the repository list that needs reminders to be sent (could be a json within this repo?)
  • cron.yml waits 24h, then triggers compute.yml
  • compute.yml runs as usual, updates the repository reminders read from the list we stored before
  • if there is nothing to be done (e.g. there is no more assignees to watch anywhere), then compute.yml disables cron.yml workflow
  • later on, if someone gets assigned, enable cron.yml workflow from compute.yml, rinse and repeat

This way, no useless runs would happen from the cron, we would properly watch repositories and use the correct configuration during runs. This would also lower our API usage significantly.

@whilefoo
Copy link
Member

whilefoo commented Jan 5, 2025

If I understand correctly every time issues.assigned triggers the plugin it would add the issue to a list and store it somewhere and on issues.unassigned remove it from the list and if the list is non-empty it would enable cron.yml that would run compute.yml every x hours, and if the list is empty it would disable cron.yml. On every compute.yml run it would check if reminders need to be sent or unassign inactive users.
The main problem I see is how would the plugin get the installation token when it's run by cron.yml?

@gentlementlegen
Copy link
Member Author

You are correct. For the installation token, we have APP_ID and APP_PRIVATE_KEY organization wide, which should enable the cron to authenticate as well?

@0x4007
Copy link
Member

0x4007 commented Jan 5, 2025

We can try your suggestion I just hope it's not too brittle but that depends on how well it's implemented

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

3 participants