8000 Future of the horizon-pool repo · Issue #169 · horizon-eda/horizon-pool · GitHub
[go: up one dir, main page]

Skip to content

Future of the horizon-pool repo #169

Open

Description

@carrotIndustries

As it's probably evident by PRs not getting merged, I don't have the time and motivation for reviewing external contributions . Let's use this issue to brainstorm for ideas on how that situation can be improved.

Making the review process easier

To easily get PRs into my clone of the pool for review I added a bit of libgit2-based code that pulls a PR into a local branch and merges it with master. Since that feature didn't meet my requirements in terms of robustness, it gathered dust on a local branch. To make this available to other people interested in reviewing PRs, I merged this into master: horizon-eda/horizon@e2d4cd2 but it needs to be enabled manually in the prefs.json

In the grand scheme of things, I see two ways to make reviewing PRs easier:

  • Enhance the pool manger in such a way that the entire PR review process can be done from within Horizon EDA by using the github api.
  • Have a bot that posts renderings of the changes to the PR's comments. Unfortunately, I couldn't figure out a way to do this without running my own server as an untrusted PR needs to trigger scripts defined in a trusted repo that has access to API keys for posting a comment.

More automation

Automatically check for things such as missing items like as PRs just containing a unit or datasheet links pointing to secondary sources. Some of this can probably be supported by the existing GitHub Actions workflow: https://github.com/horizon-eda/horizon-pool/blob/master/.github/workflows/all.yml On the long rung, I'm envisioning something equivalent to Debian's Lintian, but for packages, parts, etc.

More reviewers

To have more people than me having merge permissions, we probably need to work on the open issues on https://github.com/horizon-eda/horizon-pool-convention/ so that no matter who's reviewing the PR, we end up with the same result.

What else?

I don't believe that there's a single solution to this problem, so we probably need to do at least all of the things listed above. Any more ideas?

Activity

  1. kliment commented on Aug 31, 2020

    @kliment

    About the untrusted PR, the action functionality in github can run pre-defined tasks and produce artifacts given a docker image. A comment action can access those artifacts and post a comment. The build task doesn't need access to secrets and the API token is only accessible to the (trusted) script posting the comment. Example here: https://github.com/machine-learning-apps/pr-comment

  2. atoav commented on Aug 31, 2020

    @atoav
    Contributor

    If servers are needed, I am running multiple which I could put the scripts on. Anyways a cheap virtual Hetzner-Ubuntu box costs around 4 Euros per month, that shouldn't be the problem.

    I think we also have to talk about the organizational side of things, namely stuff like:

    • Who should be in charge of what and who is in charge of who is in charge etc
    • Who will be allowed to do what
    • What is needed for a PR to be accepted (how many people need to look at it)
    • Are reviewers allowed to accept their own PRs? While this would speed things up it is probably not a good idea
    • How and wehre is this all clearly documented
    • Once there is a process that works, having tutorials helps people to grok things

    Maybe a few things could also be done as a concerted effort (like creating and reviewing the most commonly used packages). If you know the package is alright reviewing has already become a lot easier.

    This of course all ties in with Automation. Investing a little more time on doing that properly now is probably saving many people a lot of time and energy in the long run so it is definitly something that pays off. Automation is, what would enable such clear and easy workflows. If a lot of that could become part of the pool manager itself that would be even greater. I am gladly using git from the shell to do things by hand, but I'd doubt that this is a very common thing among electronics people.

  3. fruchti commented on Aug 31, 2020

    @fruchti
    Contributor
    < 8000 /div>

    A streamlined PR reviewed process would greatly benefit the pool. I'd imagine stalling PRs without apparent reasons can easily demotivate users and are, in a way, holding Horizon back because there are not as many contributions as there could be.

    As @atoav mentions, the details need to be worked out there, but there needs to be a common, agreed upon, base for all decisions, too. Let me know if I can do anything for the convention or its issues to get the discussion there going again.

    As we are talking about processes, I also wanted to come back to the possibility of a multiple ‘stages’, ‘channels’, or however you wanted to call them, containing parts with different levels of dependability:

    Here I had left some ideas how the review process and the responsibility for part correctness could be split up among users. I think it is still relevant as it could make the reviewers' lives easier, who wouldn't have to shoulder everything, while providing more probably-correct parts for end users.

    In a nutshell, my idea was to have separate ‘testing’ and ‘stable’ pools (branches of the repository or with some built-in tagging system). Contributions could be merged into ‘testing’ with a minimal amount of sanity checks from reviewers, where they would be available to end users after an explicit opt-in. Users building their schematics and layouts with these untested parts can report back in the part's tracking issue, testifying for what they validated. As soon as every item on a PR's check list (footprint, pinout, 3D model, …) has been independently validated, it can be merged into ‘stable’.

    This could already be done today, with a ‘testing’ pool including the main ‘stable’ pool, without changes to Horizon. If we were to adopt something like this, it still might be a good idea to think about UX and possible direct integration into Horizon first, though. I would, for example, imagine a simple flag ‘confirmed’ for each unit/entity/symbol/etc.

    The current ‘PR load’ isn't anywhere near high enough that a small team of reviewers couldn't be able to shoulder it, and letting average users cross-check parts instead of just trusted reviewers might be debatable, but I think a process like this could greatly benefit the number of available parts for end users (if they are willing to cross-check everything, which most people probably do anyway).

  4. carrotIndustries commented on Aug 31, 2020

    @carrotIndustries
    MemberAuthor

    The build task doesn't need access to secrets and the API token is only accessible to the (trusted) script posting the comment. Example here

    Doesn't this just shift the issue? How does the comment-posting script ensure that it doesn't get spam to post?

    If servers are needed, I am running multiple which I could put the scripts on. Anyways a cheap virtual Hetzner-Ubuntu box costs around 4 Euros per month, that shouldn't be the problem.

    Thanks for the offer, but I try to stick with manged solutions such as github pages, actions, discourse, etc. that don't require any sysadmin work. Having though about this, a bit I think I have something that could work even though it doesn't seem particularly elegant:

    • Webhook in the horizon-pool repo
    • That triggers an aws lambda function which then in turn
    • calls the repository_dispatch handler of a workflow in a trusted repo that'll then post to the PR.
  5. kliment commented on Sep 1, 2020

    @kliment

    The comment posting script only posts a comment with a fixed template and the output artifacts from the PR. The script itself is using the version from a predefined branch rather than whatever comes in the PR. That's a weird configuration, which is why I linked to an example. If someone is motivated enough to provide their spam in the form of a horizon library item, well I guess we just have to live with that.

  6. atoav commented on Sep 1, 2020

    @atoav
    Contributor

    That could be solved as well by having the AWS labda script look at the presence of certain markers or flags that only a privileged user is allowed to add. That would make this a semi automated process though.

    I agree with @fruchti that conforming to the template is enough of a hurdle to get rid of random bots and low effort spammers. What might be more problematic would be well meaning users posting copyrighted material (e.g. 3D models), because that stuff isn't easy to figure out automatically.

    Edit: I offered my servers because I already do the sysadmin work on them, however a AWS lamda would be a good fit as well

  7. atoav commented on Sep 1, 2020

    @atoav
    Contributor

    Channels UX

    The idea to have multiple "channels" for parts makes a lot of sense, because then users won't have to wait to use their parts, while still being able to contribute. If we can manage to basically distribute the work of checking onto the users as well, that would probably be the most scalable approach. This however would mean a lot of streamlining when it comes to UX:

    • It must be easy to opt in an out of different channels
    • If parts are unchecked this must be somehow displayed
    • If someone checked a part, successfully manufactured a board or found a fault, this shouldn't be more than a few clicks as well (and in the backend we could just keep a tally of the number of checks and successful usages)
    • Maybe it makes sense to list "my parts" – so stuff people contributed to build a sense of ownership
    • The process of moving stuff from one channel to another must be seamless
    • The process of updating pools and updating projects that depend on it must be seamless as well

    I also had to think about the way LibrePCB does this:
    library_manager
    This is obviously a different concept given the different structure, but I think it is well worth it thinking a little bit about what the usability pitfalls with using multiple pools at the moment are and grinding down the edges that exist.

    Because the experience with multiple pools as for now is like this:

    1. open horizon, see one or more pools
    2. create a new pool that is meant to be included in the official one and edit it
    3. realize you won't be able to depend on packages etc from the official pool
    4. ???

    In a lot of cases it might be beneficial if the included/child pool could inherit from the parent or am I overlooking something? As for now parts/packages/etc of included pools are colored differently in the pool editor – what if one would be able to select whether a new part/package/etc should be created in the parent or one of the child pools?

    One problem I see with that, is that the child pool doesn't know it is a child of anything, so if one were to edit it separately it would probably scream in horror about missing references.

    Git integration

    I think the current git integration would need an major overhaul as for now it "works", but a lot of things are missing (e.g. ability to review PRs as mentioned). When I click the Git tab on my pool, the program freezes for 15 seconds, my fan starts to spin and then eventually I see something.

    I know the whole thing is probably a bit of an hairy issue, that isn't too much fun to work on, but It is definitely something that should be tackled given the role it plays for the development of the whole pool ecosystem. And precisely because of the hairy nature having vision and some sort of goal to strive for is needed.

  8. fruchti commented on Sep 1, 2020

    @fruchti
    Contributor

    What might be more problematic would be well meaning users posting copyrighted material (e.g. 3D models), because that stuff isn't easy to figure out automatically.

    Very good point. There are also bots which make agreeing to the licence a prerequesite for contributions, i.e. contributers would have to sign they have the rights to redistribute their 3D models under the pool's licence. As an example, this one is the first one I came across: https://github.com/finos/cla-bot

  9. carrotIndustries commented on Sep 1, 2020

    @carrotIndustries
    MemberAuthor

    The comment posting script only posts a comment with a fixed template and the output artifacts from the PR. The script itself is using the version from a predefined branch rather than whatever comes in the PR. That's a weird configuration, which is why I linked to an example

    I'm probably still not getting it: We add a workflow to this repo that gets triggered by forked PRs and doesn't have access to any useful secrets (the github_token is readonly), so how is this workflow supposed to to anything privileged such as commenting on a PR?

    No matter how it's going the be deployed, I think that a bot that does as much linting as possible is something that'd help a lot as the contributor will get immediate feedback on their PR and some common mistakes can be caught without opening the pool manager.

    When I click the Git tab on my pool, the program freezes for 15 seconds

    You should probably create an issue for that. Is any of the two lists particularly long?

    Re multiple pools: One workflow that's currently supported, but not really intuitive is to create a pool for each project and mix-and-match pools from other users.

    As far as the git integration goes, for users not familiar with git, there's the "remote" tab that'll appear when downloading the pool using the pool manager.

  10. kliment commented on Sep 2, 2020

    @kliment

    I'm probably still not getting it: We add a workflow to this repo that gets triggered by forked PRs and doesn't have access to any useful secrets (the github_token is readonly), so how is this workflow supposed to to anything privileged such as commenting on a PR?

    The workflow gets triggered by PRs. Said workflow has access to secrets, and can do privileged stuff, but you cannot modify the workflow itself from a PR without approval so it can neither leak secrets nor do anything you haven't preconfigured. The key thing is that the workflow is specified to use a particular version or branch when it's triggered. Separately from this, you run a docker image (that you control) that also cannot be modified from a PR without approval, that does whatever you like in the background linting and rendering and all, and produces a set of artifacts (images, error/warning lists, whatever) that has access to the PR's changes. Your commenting workflow gets access to these outputs, and can incorporate them into a comment in any way it wishes. The PR author has read-only access to the workflow script, read-only access to the docker image, and no access to the secrets. The workflow has access to the docker thing's output, and posting rights. The docker thing has code you control and data coming from the PR. Of course, both the workflow itself and the docker image it uses can be changed by a PR, but those changes are not effective (do not get run) until and unless a person with commit access approves them. The do get run in the submitter's repo, but don't have access to your repo's secrets.

    Hope this helps - like I said it is confusing, this the example implementation

  11. carrotIndustries commented on Sep 2, 2020

    @carrotIndustries
    MemberAuthor

    The workflow gets triggered by PRs. Said workflow has access to secrets, and can do privileged stuff, but you cannot modify the workflow itself from a PR without approval so it can neither leak secrets nor do anything you haven't preconfigured.

    As far as I understand workflows, they can be modified by PRs as well. The docs also state that the github token is read-only for forked PRs: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-events-for-forked-repositories Secret's aren't passed either: https://docs.github.com/en/actions/configuring-and-managing-workflows/creating-and-storing-encrypted-secrets#using-encrypted-secrets-in-a-workflow

  12. kliment commented on Sep 2, 2020

    @kliment

    From your link: When you create a pull request from a forked repository to the base repository, GitHub sends the pull_request event to the base repository and no pull request events occur on the forked repository.

    The example I linked responds to that event - in our repository, not the forked one. And then we can do all the things we want. GITHUB_TOKEN is passed to the runner, but the runner runs in our context not the forked repo's. GUTHUB_TOKEN gets passed to workflows that are triggered by PRs, but the repo where the PR is coming from cannot access our GITHUB_TOKEN. And of course a PR can modify the workflow, but only once it's merged. Pre-merge the workflow that gets run is the workflow that's in the base repo, even though it gets runs on the changes in the PR. So here is what happens:

    1. Attacker forks repo at commit 0xg00dc0de, makes changes to workflow in commit 0xbadc0de, sends pr
    2. Our workflow in the base repo gets run, triggered by a pull_request event
    3. Our workflow triggers a docker image that runs the pool manager and has access to the new commit's data, and generates a bunch of files as output
    4. Our workflow then triggers a script (which can live in a separate repo or branch) that downloads said files, and uses GITHUB_TOKEN to post a comment with them. The docker image never seen GITHUB_TOKEN and the pr author never sees anything except the result
    5. We look at the commit that does evil things, and reject it
      So the workflow that gets executed is always one we control, and the code that accesses GITHUB_TOKEN is under our control and not the same code that accesses the attacker-controlled data.

    Yeah, I know it's confusing and underdocumented. But it does actually allow a sensible workflow.

  13. carrotIndustries commented on Sep 2, 2020

    @carrotIndustries
    MemberAuthor

    Is just found https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ The pull_request_target event does exactly what I need without any Rube-Goldberg contraptions as it uses the workflow defined in the repo rather than the one from the merge commit.

  14. carrotIndustries commented on Sep 2, 2020

    @carrotIndustries
    MemberAuthor

    I tested pull_request_target and it works as advertised: carrotIndustries/horizon-pool-test#10

  15. 31 remaining items

  16. carrotIndustries commented on Mar 14, 2021

    @carrotIndustries
    MemberAuthor

    Is this the main problem here?

    It also makes pool updates slower. I've already played with putting the SQLite action into its own thread, but figured the complexity wasn't worth it yet.

    At most, there could be separate THT and SMT passives pools because you’d likely only need one.

    Chances are, one also doesn't need passives from every manufacturer under the sun.

    Another idea: the part browser could have inverse filtering, too, perhaps by tag.

    Sound reasonable, any idea on how the UI should look like?

    but it’d be very nice to have a way to share ‘niche’ parts without clogging the main pool.

    I'm glad you asked since the latest commit added just that, though there's still some copy writing to be done: Add your pool to https://github.com/horizon-eda/horizon-pool-index/tree/master/pools with the level set to community. Once it's merged an action will pick it up and update https://pool-index.horizon-eda.org/index.json accordingly. This list is then used by the new pools window:

    2021-03-15-001326_832x409_scrot

  17. RX14 commented on Mar 15, 2021

    @RX14
    Member

    Any first idea on what the boundary would look like for "too niche to be in core"?

  18. atoav commented on Mar 15, 2021

    @atoav
    Contributor

    @RX14 As one of the main oddball contributors (it appears) I'd guessavailability plays a huge role in this. So if the part is only available at a weird chinese Aliexpress reseller, it might be oddball. If the part is not produced anymore but there is still stock being sold (e.g. certain germanium tranistors), it is quite definitly oddball. If it is very special purpose and not being sold on the typical reseller sites (Mouser/Digikey/...) it might be oddball as well (e.g. Coolaudio V2044A).

    Drawing the line here will always be subjective I guess.

    @carrotIndustries I would consider breaking out all of my audio stuff into an Audio/Synth pool. How would we deal with shared things like symbols? E.g. if I create a OTA Symbol/Unit/Entity for say a LM13700, this would probably be something that should be in the main pool as well? Should I add it via PR to both pools then?

    On the issue of inverse filters: I think I proposed somethign similar a while back when I said maybe it would make sense to allow users to configure a set of global exclude/include filters in the preferences. This way if someone feels particularily annoyed by a subset of parts, they could a narrow down with filters what they see on their day to day usage of the software. Maybe adding a visible switch to the part browser to temorarily disable that filter makes sense as well, but I think these filters should be in the preferences and they should be persistent between restarts of the software (as opposed to being visible in the part browser and non persistent between restarts).

  19. carrotIndustries commented on Mar 15, 2021

    @carrotIndustries
    MemberAuthor

    E.g. if I create a OTA Symbol/Unit/Entity for say a LM13700, this would probably be something that should be in the main pool as well?

    That part in particular could go into the main pool as it's readily available at digikey and other reputable distributors.

    Any first idea on what the boundary would look like for "too niche to be in core"?

    I think it makes more sense to decide this based on the part's availability. If a part is particularly hard to get or only available on ebay or so, it shouldn't be in the main pool.

  20. fruchti commented on Mar 23, 2021

    @fruchti
    Contributor

    Another idea: the part browser could have inverse filtering, too, perhaps by tag.

    Sound reasonable, any idea on how the UI should look like?

    How about a filter exactly like the ’Tag’ filter, just called ‘Exclude tags’? It would be pre-populated with ‘resistor’ + ‘capacitor’ (+ ‘inductor’?). I’d rather not put very generic/vague tags like ‘passive’ in there by default so users

    • can remove specific exclusions when looking for e.g. an inductor, and
    • aren’t surprised when the search doesn’t bring anything up because it isn’t clear what parts are tagged ‘passive’.

    Maybe ‘With tags’ + ‘Without tags’ would be a clearer labelling, not sure there.

  21. carrotIndustries commented on Apr 3, 2021

    @carrotIndustries
    MemberAuthor

    I've created a reviewer team that can add tags to PRs, as of now, it's @RX14 and @sh-ow. Who else wants to take part? @fruchti , @jue89, @atoav ?

  22. jue89 commented on Apr 3, 2021

    @jue89
    Contributor

    Sure! I can't make any promises regarding the amount of time I can spend for reviews, but I'd love to help out whenever I find some time. The review guidelines are described in the horizon-pool-convention repo, right?

  23. fruchti commented on Apr 3, 2021

    @fruchti
    Contributor

    Same for me, sign me up!

  24. atoav commented on Apr 3, 2021

    @atoav
    Contributor

    Ah sure thing, i am all in

  25. carrotIndustries commented on Apr 5, 2021

    @carrotIndustries
    MemberAuthor

    The recent commits move all of the generated passives to their individual repos:
    2021-04-05-225116_1122x696_scrot

  26. B48D81EFCC commented on Jan 22, 2026

    @B48D81EFCC

    Hi,

    Is this pool repository dead?
    No pull requests have been completed since the end of 2024.

    I'm asking because I opened two pull requests. If this repository is unmaintained, however, I won't submit any more (since they'll never be completed).

  27. RX14 commented on Jan 22, 2026

    @RX14
    Member

    Right now it is, but I'd like it not to be. We've had a somewhat high bar for review historically, and I think we have to let that go.

    I'd like to change the standard so we merge PRs of anything which passes DRC, is complete, and has been validated by being the board being assembled correctly.

  28. tkornack commented on Jan 22, 2026

    @tkornack
    Collaborator

    There probably should be some basic quality bar wherein one of us looks at the submission and approves.

  29. B48D81EFCC commented on Jan 22, 2026

    @B48D81EFCC

    Thanks for your quick reply and the clarification.

    I’d be happy to help with reviewing of contributing parts to the repository.
    However, I’m still getting familiar with the review process and the specific requirements (e.g., DRC checks, validation workflows). As you can see #347, I am still struggling a bit.

    If you’re open to it and think it would help, I’d be grateful for some guidance or pointers to relevant documentation to get up to speed.

  30. RX14 commented on Jan 22, 2026

    @RX14
    Member

    There probably should be some basic quality bar wherein one of us looks at the submission and approves.

    Yes, I agree, reviewers should reject things which are obviously wrong. However I want to avoid spending a long time digging through datasheets to validate footprints and pinouts, and instead go with a simpler test: does it work on a real board. If it passes that threshold, then it's clearly useful to someone and I believe belongs in the library.

    If you’re open to it and think it would help, I’d be grateful for some guidance or pointers to relevant documentation to get up to speed.

    This is the second part of the problem, there are no links or references to the actual standard, and even that is not very beginner friendly.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions

      0