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

Feat/config installer #13

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Nov 9, 2024

Resolves #3
Requires #11 and #12 (reviewing those could be skipped and focus placed just on this PR)

  • Listing all plugins from ubiquity-os-marketplace and grabbing manifests
  • Login with GitHub App: fetch user orgs with app installs.
  • Select org: Select config: Select Plugin
  • Update configuration based on manifest values
  • Save to local and optionally push to GitHub
  • Can build multiple plugins before pushing to GitHub

Still buggy with some plugins I need to inspect the more complex ones and handle their configs better but this shows what it looks like with a plugin with a medium size config. It should be ready to over the next day.

@zugdev Idk if you want to work your magic after this is merged, branch off and PR against dev or into this before that or just review and create a spec for what's needed? What do you think?


We can still build this UI with the ability to read from a query param directly too but it's a cleaner approach housing them all imo.

  • I'm thinking pull the plugin README and display it beside the configuration?
  • breadcrumbs would be good to jump back and re-select plugin, config, org.
  • it's pretty verbose but additional details in the manifest for each prop. In .ts we use JSDoc comments on input props and it fills the IDE. This would be handy af if we had a workflow to parse these and write them to the manifest which could be read on this UI: e.g requiredLabelsToStart without defaults might be confusing to a partner

QA:

plugin-installer-demo.mp4

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 9, 2024

  1. manifest.name should reflect the repository name so we can match it to the worker/action url
  2. all plugin manifests rendering without error.
  3. redesigning the UI to accommodate the extensive configs should be spec'd
  4. I've achieved the spec as it exists for this task, following some minor tweaks to config prop options this is review ready.

QA with official plugin URL:

demo-sbs.mp4

@zugdev I saw you say that this codebase is your scope, perhaps you want to start reviewing my PRs as I think 0x4007 is busy.

@Keyrxng Keyrxng mentioned this pull request Nov 9, 2024
.cspell.json Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@zugdev
Copy link

zugdev commented Nov 10, 2024

I will review UI, yes. Regarding my contribution, I think it makes more sense for this to be merged first and only then implement styling.

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 10, 2024

@gentlementlegen, is null valid for text-conversation-reward inputs for the modules? It appears so via the schema, is this valid to ship for V1 and we can decide on how we'll display all of the HTML tags?

image

image

@gentlementlegen
Copy link
Member

@Keyrxng Yes it is, it means the module is disabled.

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 10, 2024

@Keyrxng Yes it is, it means the module is disabled.

Understood.

That's the kinda thing I'm trying to think how can we tell the partner that for any prop on any plugin?

My first thought is to capture /**\ comments above the pluginSettingschema values or something and inject them as a description into configuration when it's auto-generated via wf.

Or present them the readme when they select a plugin which should say null === disabled or whatever somewhere?

Any better ideas?

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 10, 2024

Alright well I'm going to grab QA of:

  • adding a plugin
  • removing a plugin
  • adding multiple plugins

Is there anything else I need to do to get this merged, spec was minimal but it has been achieved:

  1. Org selection > config selection > plugin selection > edit > push
  2. Our config is parsed, their config is parsed and their config is updated.
  3. No LLM usage required.

@zugdev
Copy link

zugdev commented Nov 10, 2024

Actions doesn't seem to be building this PR and therefore no deployments are available. 0x4007 just merged template into this repo, so pulling from main should solve it, I want to help you QA it.

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 11, 2024

Actions doesn't seem to be building this PR and therefore no deployments are available. 0x4007 just merged template into this repo, so pulling from main should solve it, I want to help you QA it.

Actions are failing due to env vars which I cannot edit I'm afraid. Jest is failing due to the old octokit/core imports as usual, since tests are not written for this codebase yet we can either ignore CI or I can remove the template test suites until unit tests are built.

Also afaik preview deployments are for reviewers that aren't able to QA locally like 0x4007 from mobile etc. This does run locally if you'd like to QA it.

@Keyrxng Keyrxng mentioned this pull request Nov 11, 2024
@zugdev
Copy link

zugdev commented Nov 11, 2024

Yes, deployments are a mere facilitator, I understand the circumstances. I'll be testing it locally soon!

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 11, 2024

Yes, deployments are a mere facilitator, I understand the circumstances. I'll be testing it locally soon!

LFFGGG

Some points for setup:

  1. Your DB auth should be tied to a GitHub App not an OAuth app. The kernel is a GitHub app, the same is required for this because we need permissions that are out of scope for a normal OAuth app.
  2. For QA it's probably best to just use your kernel app and update the callback url like I did. For prod I'm unsure right now if we'll need a new app for this or if it's safe to use the kernel.

Known problems:

  • sign in sometimes will happen on first try, others it will accept the oauth response but not appear signed in, another click to sign in will register.
  • not all plugins can be installed right now because of their schema: validation errors regarding null inputs of required props which we cannot easily display the options to change. This requires the UI be refactored to accommodate such large amounts of config options
  • not all plugin' manifest.name reflects their repo and we need these to match so we can track their deployment url (I'll PR each plugin that needs updated once merged)

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 11, 2024

  1. Modify an installed plugin
  2. Remove an installed plugin
  3. Add multiple plugins (config stored contained both the newly added and the previously removed as it was not cleared from state for sake of QA)

I decided to display objects via JSON.stringify and modify the object vals directly vs my initial approach in creating inputs for each.

add-remove-add-multi.mp4

@Keyrxng
Copy link
Member Author

Keyrxng commented Nov 11, 2024

I'm going to move on and complete other tasks while reviews are pending

@zugdev
Copy link

zugdev commented Nov 12, 2024

If I sign in with an account that has nothing to show, we should tell that to the user.

image

The auth token is not being cleared on sign out, the only way for me to currently logout is to clear my cookies manually.

Screen.Recording.2024-11-12.001120.mp4

Also we have a scope error that is not allowing it to fetch my orgs correctly, I've pinpointed the issue in my review comments:

image

zugdev

This comment was marked as resolved.

@zugdev
Copy link

zugdev commented Nov 12, 2024

I think you should try catch whatever can fail, that certainly includes HTTP requests. For instance, to get the list of app installations you need an authenticated GitHub app. I couldn't login with my org so I got deadlocked. We should at least show a message in those cases.

@zugdev
Copy link

zugdev commented Nov 12, 2024

Some points for setup:

  1. Your DB auth should be tied to a GitHub App not an OAuth app. The kernel is a GitHub app, the same is required for this because we need permissions that are out of scope for a normal OAuth app.

Ignore what I previously said about auth, already hid my comments, my Supabase is set for OAuth - my bad. However I still believe there should be try catches.

How should I set this up? I don't see a way to do it in Supabase's dashboard. Or did you mean signing in with a GitHub app? If that's the case, I wasn't able to.

@@ -9,15 +11,56 @@ async function handleAuth() {
if (!token) {
// await auth.signInWithGithub(); force a login?
Copy link

Choose a reason for hiding this comment

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

Don't force a login, just display a default message

public async getGitHubUserOrgs(): Promise<string[]> {
const octokit = await this.getOctokit();
const response = await octokit.request("GET /user/orgs");
Copy link

Choose a reason for hiding this comment

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

This is failing, it seems that the correct endpoint for this request would be "GET /organizations", based on this piece of docs: https://docs.github.com/pt/rest/orgs/orgs?apiVersion=2022-11-28#list-organizations

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.

Create add/remove config logic
3 participants