Conversation
metalwarrior665
left a comment
There was a problem hiding this comment.
This looks simpler than I thought :) I have just minor remarks, feel free to argue against them. Let's wait for Juan but this seems quite uncontroversial.
| for (const [index, configEntry] of configs.entries()) { | ||
| const { folder, actorFullName } = configEntry as ActorGlobConfigEntry; | ||
|
|
||
| // TODO: Allow for combined filtering? |
There was a problem hiding this comment.
I would allow this, don't think it is that confusing or dangerous. You simply filter once per folder and then per name. There might be legit use-cases for this
| import { selectActors } from './actor-filtering.js'; | ||
| import { isPathWithinScope } from './path-utils.js'; | ||
| import type { ActorConfig, ActorConfigFile } from './types.js'; | ||
| import type { ActorConfig, ActorConfigFile, ActorConfigFileEntry, ActorGlobConfigEntry } from './types.js'; |
There was a problem hiding this comment.
These types don't match very well, one says "File", other doesn't.
btw we already have ActorConfig type which is basically the same thing, we should either unify them or derive one from the other. Are there cases where these will differ? If we are simply merging them then they should not differ. No need to solve that in this PR but sooner rather than later.
|
|
||
| const validateGlobConfigEntries = (configs: unknown[]): ActorGlobConfigEntry[] => { | ||
| for (const [index, configEntry] of configs.entries()) { | ||
| const { folder, actorFullName } = configEntry as ActorGlobConfigEntry; |
There was a problem hiding this comment.
Could be zod parse I guess but I don't know if you can get such a nice errors from it, don't have experience
There was a problem hiding this comment.
I also don't have much experience with zod but when I looked into it I remember that there was a lot of overhead to make it work. I don't think it's necessary, as long as the inputs here are somewhat limited in number
|
|
||
| let overlay: Partial<ActorConfigFileEntry> = {}; | ||
| for (const configEntry of [...matchingFolderConfigs, ...matchingActorFullNameConfigs]) { | ||
| const { folder: matchedFolder, actorFullName: matchedActorFullName, ...rest } = configEntry; |
There was a problem hiding this comment.
Let's think a bit if we shouldn't separate the matching fields folder, actorFullName vs the configs, it is a bit weird they are on the same level
| ); | ||
|
|
||
| let overlay: Partial<ActorConfigFileEntry> = {}; | ||
| for (const configEntry of [...matchingFolderConfigs, ...matchingActorFullNameConfigs]) { |
There was a problem hiding this comment.
If we want to support having both folder and name with AND logic, this would need to change
| const matchingActorFullNameConfigs = configs.filter( | ||
| (configEntry) => | ||
| configEntry.actorFullName !== undefined && | ||
| typeof actorEntry.actorFullName === 'string' && |
There was a problem hiding this comment.
We already validate this eariler and the type should be string | undefined now, no?
|
In this PR I postponed the problem that would pose adding the notifier object. As of now, an object would be completely rewritten and it does not have a partial merging logic (e.g. every notifier would get the same token but each actor needs to get its own slack channel for test report). I gave it some thought and even though it's not necessary as of this PR would be merged, we want to add it soon so it makes sense to do it here, so that the diff is tidy. @metalwarrior665 I'll do your requested changes first and then I'll try to add the deep object merging logic. |
| 1. entries matching on `folder` alone | ||
| 2. entries matching on `actorFullName` alone | ||
| 3. entries matching on both `folder` and `actorFullName` together |
There was a problem hiding this comment.
This might be more complicated than just order based
| const validateGlobConfigEntries = (configs: unknown[]): ActorGlobConfigEntry[] => { | ||
| for (const [index, configEntry] of configs.entries()) { | ||
| const { folder, actorFullName } = configEntry as ActorGlobConfigEntry; | ||
| const { match, set } = configEntry as ActorGlobConfigEntry; |
There was a problem hiding this comment.
I would give this 2nd thought; maybe it is unnecessarily nested now just for the 2 matcher fields.
There was a problem hiding this comment.
Actually I quite like the assertiveness of match and set
There was a problem hiding this comment.
i was thinking of something like Record<glob, config>.
ohhhh your matcher can do both folders and names, makes sense actually.
Yeah, match and set do it for me.
| 3. entries matching on both `folder` and `actorFullName` together | ||
| 4. the actor's own literal entry in `actors[]` | ||
|
|
||
| Within a single tier, if more than one entry matches, only the _last_ one (array order) applies — earlier same-tier matches are dropped entirely, not merged in. Across tiers, results are deep-merged from lowest to highest precedence: a higher tier wins on any field it sets, but a field it doesn't set is inherited from a lower tier rather than being lost. Object-valued fields merge key by key; array-valued fields (e.g. `overrideActorContext`) keep the higher tier's array intact and append only the lower tier's entries that aren't already present, preserving the higher tier's order. |
There was a problem hiding this comment.
This is too complex. I would drop the notion of tiers completely. 1-3 can just be taken in order (any use-case for the current way?). And the point 4 can stay undocumented (for backward compat) and we just append it at the end of the glob array so it will override any conflicting fields before.
only the last one (array order) applies — earlier same-tier matches are dropped entirely, not merged in
I don't get this. I thought the whole goal is to merge here. E.g. one glob sets token on Actor X, another glob sets slack on Actor X etc., these are merged in the config object. Theoretically, we could only merge the top level config fields but I'm not sure about this
array-valued fields (e.g.
overrideActorContext) keep the higher tier's array intact
I think we should not merge arrays. They shouldn't include more nested objects so there won't be need for any deep merging. Users can just retype the whole array in the later glob.
There was a problem hiding this comment.
Alright, I will just do it by order, it's much easier to understand, you are right, I was just hooked on the "more specific" idea, especially with the filter combo.
I would concede the arrays overwrite (I don't see a particular use for now, other than avoid typing a specific folder that goes into multiple overrideActorContext) but I would keep the deep merge and not to just top level. You could have:
"configs": [
{ "match": { "folder": "**" }, "set": { "notifiers": { "slack": { "tokenEnvVar": "SLACK_TOKEN" } } } },
{ "match": { "folder": "actors/*" }, "set": { "notifiers": { "slack": { "targets": { "release-report": { "dev": "#releases" } } } } } },
{ "match": { "actorFullName": "myteam/web-scraper" }, "set": { "notifiers": { "slack": { "targets": { "test-report": "#ws-tests" } } } } }
]and this will allow to avoid resetting the token every time and still give flexibility on the other options.
| return undefined; | ||
| }; | ||
|
|
||
| const validateGlobConfigEntries = (configs: unknown[]): ActorGlobConfigEntry[] => { |
There was a problem hiding this comment.
this whole function is just one zod schema 👀
It would probably take wayy less mental overhead and probably be parsed better 😅
| // matching combined (folder AND actorFullName) entries, the actor's own literal entry. The tier | ||
| // winners deep-merge in that ascending order, so a field one tier doesn't set is inherited from | ||
| // a lower tier instead of being clobbered. | ||
| export const mergeGlobConfigs = ( |
There was a problem hiding this comment.
fuck this is complicated, are the tiers even necessary?
Also just had a long chat with some guys from the team and now I'm pretty sure this config scheme is not the way to go 😭.
I'll think a bit more about this today and get back to you
There was a problem hiding this comment.
We agreed to remove the tiers here. But happy to hear the arguments from the outside.
332a437 to
6d424ba
Compare
JuanGalilea
left a comment
There was a problem hiding this comment.
code is too inlined for my taste
Also i have some comments on functionalities.
Seems like the groundwork for modes payed off, since it seems we can add these pretty easily
There was a problem hiding this comment.
add one test here parsing one with globs, so as to test that the whole thing is doing what its supposed to.
Its slightly redundant but still valuable imo.
I did 1 with grouped somewhere here.
| z.object({ | ||
| folder: z.string(), | ||
| actorFullName: z.string().regex(ACTOR_FULL_NAME_REGEX), | ||
| tokenEnvVar: z.string().optional(), |
There was a problem hiding this comment.
THEY CAN HAVE STUFF HERE?
whats the precedence?
Also, nice for legacy compat, but slightly worrying
There was a problem hiding this comment.
There is no precedence, if the configs try to set something that it is already there, it errors. It's why there is the owner that check who set the key
There was a problem hiding this comment.
yeah, saw it in the implementation. I think its good.
| folderGlob: z.string().optional(), | ||
| actorFullNameGlob: z.string().optional(), | ||
| }) | ||
| .refine((data) => Object.keys(data).length > 0, 'Invalid input: At least one glob is required.'), |
There was a problem hiding this comment.
i hate that this is the zod standard for this kind of thing.
| const errors: string[] = []; | ||
|
|
||
| const resolved = body.actors.flatMap((actor): ResolvedActorConfig[] => { | ||
| const result = { ...actor }; |
There was a problem hiding this comment.
structuredClone might be safer if we ever get some nesting on the actor fields
| export const GLOBS_PARSER = defineStrategy(CONFIG_FILE_STRATEGY.GLOBS, schema, (body: GlobsConfig) => { | ||
| const errors: string[] = []; | ||
|
|
||
| const resolved = body.actors.flatMap((actor): ResolvedActorConfig[] => { |
There was a problem hiding this comment.
holy function nesting batman!
inline function that inlines a function into flatmap? 👀
The flatmapped one might benefit of some testing without all the other fluff man
| ); | ||
| } else { | ||
| errors.push( | ||
| `Actor "${actor.actorFullName}": "${key}" is set by both configs[${owner}] and configs[${index}].`, |
There was a problem hiding this comment.
having configs be an array of rules make these errors very painful to write 😭. Maybe a shortened "selector" view could be nice.
| errors.push( | ||
| `Actor "${actor.actorFullName}" has no tokenEnvVar: set it on the actor or through a matching config.`, | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
some Either return could be beneficial here, with the added bonus of no mutation of error (not very functional-pilled my man 😂 )
There was a problem hiding this comment.
tracking useless globs (they are not owner for no key) might be beneficial to steer people out of dumb configurations
There was a problem hiding this comment.
splitting the matching function into a pure function taking actor + globConfigs[] might be good for testability
| tokenEnvVar: z.string().optional(), | ||
| overrideActorContext: z.array(z.string()).optional(), | ||
| }) | ||
| .refine((data) => Object.keys(data).length > 0, 'Invalid input: At least one setting is required.'), |
There was a problem hiding this comment.
we should do a non-empty object refining function as a zod utility.
This call is strike 3 on the duplication side (i did it once and you have 2 more)
86cfc48 to
a4dd551
Compare
Closes #128
Adds a
globsconfig file mode: actors are declared once, and shared settings are applied to them through glob-matchedconfigsentries.configsneeds at least one entry. Each entry is{ match, set }:matchtakesfolderGloband/oractorFullNameGlob(at least one). When both are given, both must match.settakestokenEnvVarand/oroverrideActorContext(at least one).minimatchwith default options.tokenEnvVar, either on the actor or from a matching config.Known limitations, opinions welcome:
actors/foo/) or a root folder ("",.) may not match the glob you'd expect.dotoption is off.