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

Simplify bridge interface #3830

Merged
merged 6 commits into from
May 3, 2023

Conversation

logan-anderson
Copy link
Contributor

@logan-anderson logan-anderson requested a review from a team as a code owner April 19, 2023 18:40
@linear
Copy link

linear bot commented Apr 19, 2023

ENG-916 Spike on simplifying Bridge

The Bridge is doing a few too many things, and is probably not a great abstraction as it stands. Corey from Codenation is doing self-hosting and separate content repos and has a need for the Github bridge, but we removed it because we'd simplified things a bit.

As we discussed some of the pitfalls with the bridge, the main area where it seems awkward is that one bridge (the FilesystemBridge) writes our config files, and the other (GithubBridge) does not. It's clear that the only time config files are being written is via the CLI, so we discussed maybe allowing the CLI to control this, and simplifying the Bridge interface.

This spike should try to see how simple we can make the bridge, so that's something that can be built upon for external users as needed (eg. a Gitlab bridge)

Can we:

  • Remove putConfig
  • Remove supportsBuilding

@changeset-bot
Copy link

changeset-bot bot commented Apr 19, 2023

🦋 Changeset detected

Latest commit: c617816

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@logan-anderson logan-anderson marked this pull request as draft April 19, 2023 18:41
@kldavis4
Copy link
Member

@logan-anderson here's my proposal for getting rid of that updateConfig handler from the database: #3842

…tion

refactor: pull out the lookup map creation so the database / bridge don't need to be concerned with it
@logan-anderson
Copy link
Contributor Author

@kldavis4 That makes sense to me. I merged it in.

jeffsee55
jeffsee55 previously approved these changes Apr 24, 2023
Copy link
Member

@jeffsee55 jeffsee55 left a comment

Choose a reason for hiding this comment

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

Small comment but no big deal, looks good

@logan-anderson logan-anderson marked this pull request as ready for review April 26, 2023 17:04
@logan-anderson
Copy link
Contributor Author

@kldavis4 and @jeffsee55 I updated the ConfigManager based on @jeffsee55 Feedback.

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

Successfully merging this pull request may close these issues.

3 participants