Closed whilefoo closed 1 year ago
@pavlovcik can you take a look?
If it's stable let's just merge it and I'll continue working on it on my branch
ok lets merge then
From the Telegram conversation
🖼 https://github.com/ubiquity-whilefoo/ubiquibot-config/blob/main/.github/ubiquibot-config.yml
i cross referenced your org config. its not clear to me how to enable the slash commands
ill dig through the source code in a bit
🖼 let me know how to fix my config
https://github.com/pavlovcik/ubiquibot-config/blob/development/.github/ubiquibot-config.yml
https://github.com/pavlovcik/ubiquibot/issues/40#issuecomment-1809371370
chatgpt says my yaml is correct so perhaps your code isnt properly implemented
also it should be throwing errors if my config is wrong so you need to restore that capability
also i dont think you're loading from the ubiquibot-config-default.ts anymore? that seems wrong
i think i found the issue:
i had an almost empty repo config. im pretty sure that becuase i had no properties for commands, it overwrote and set them all to false. of course this must be fixed.
i just deleted the local repo config and now the start command seems to work.
🖼 also trying to understand why wallet is false when its set to true in the config
# I do not understand how to respond to that command
Some additional points:
.env
or GitHub Secrets. It doesn't make sense to expose the OpenAI key in the config. # I do not understand how to respond to that command
# I do not understand how to respond to that command
🖼 also trying to understand why wallet is false when its set to true in the config
The problem was that repo and org were validated and defaults were added automatically and after that the lodash merged them so repo defaults overwrote org properties, so the fix is to first merge the configs and then validate and add defaults.
However I'm still not sure if lodash can merge arrays correctly because I don't know how it chooses the key to merge by or it just joins them, so I propose we change it to:
commands:
start: true
stop: false
OR
commands:
start:
enabled: true
# ...other future properties
stop:
enabled: false
# ...other future properties
This will also prevent duplicates and Typescript can detect if some are missing.
Perhaps we shouldn’t use lodash if it has this limitation. We should aim to make the configuration expressive and partner friendly.
I think it’s more intuitive to simplify further such as
commands:
| start
| stop
| query
etc. which to my understanding represents an array of strings.
It seems it replaces by index. So we have to find another library that handles this or make our own version
Actually lodash has a mergeWith function that accepts a custom function, I'll try that first
I can make it merge object arrays by a certain property such as name
but the problem is that the algorithm needs to be generic thus any array will need to have the same field. Or I can make it that repo arrays completely overwrite the org arrays without trying to merge them.
For string arrays I can just merge them and remove duplicates.
Taking a step back, I feel the only part of the configuration that is a headache to deal with / is not super intuitive are the commands being enabled.
Perhaps we can focus on simplifying or taking a different approach related to those?
Otherwise I think that object assign should be pretty much fine for all primitive values. For example, if a key exists on an org and repo config in the same place, we clobber and take the repo config string.
I wonder what structure makes the most sense for commands. I suppose that an array of strings isn't a primitive data type.
This is verbose but what about new root-level property names that are the command names?
Another idea is that we take the opposite approach with our commands and enable all by default BUT have disables on the repository (or even org) level?
In this case we can have that array of strings:
disabled:
| start
| stop
| query
Given that we rarely disable commands, I feel that this might be the right approach.
Not to complicate things further but when I disabled on ubiquibot the other week, I wished that we were able to also leave an automated message for why to efficiently explain to the contributors. That structure may look more like this:
disabled:
start: A large refactor is happening at https://github.com/ubiquity/ubiquibot/pull/644 please standby until this is completed.
query: User privacy is prioritized in this organization.
If we use disabled
then commands disabled in org config can't be enabled in repo config (you can only disable more commands).
Maybe instead of trying to merge we just overwrite the whole array, so if repo doesn't have disabled
array it will use the org's array but if it does then it will ignore org's array and just use repo
Another case with this problem are the labels because they are array of objects:
labels:
time:
- name: a
- name: b
priority:
- name: a
If we use disabled then commands disabled in org config can't be enabled in repo config (you can only disable more commands).
I think this might be okay. At least for our purposes. I can't think of a time where I needed the capability to selectively enable for a repository while the command was disabled on all the others.
Maybe instead of trying to merge we just overwrite the whole array, so if repo doesn't have disabled array it will use the org's array but if it does then it will ignore org's array and just use repo
From the partner's perspective this does not seem intuitive.
The last idea I have for now is to manage this in Supabase with a new command. This is probably reasonable to do if we can't find a good, partner friendly, config schema for these commands.
However I have mixed feelings on it because I think it makes the most sense to be able to manage the repository settings in one place (instead of now potentially three places- two yml and one database)
# I do not understand how to respond to that command
# I do not understand how to respond to that command
I still have some issues because of circular dependencies