Allow "additive" queries in workflow by prefixing with "+" #165
Conversation
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Generally LGTM, though I'll hold off on approving as there are going to be a lot of changes when you merge/rebase this on main.
src/config-utils.ts
Outdated
| // indicating that they shouldn't override the config queries but | ||
| // should instead be added in addition | ||
| function shouldAddConfigFileQueries(): boolean { | ||
| const queryUses = core.getInput('queries'); |
There was a problem hiding this comment.
This will need merging/rebasing on main. Now this part of the codebase has been converted to the 3rd party runner you can't use core.getInput in shared code. This input has to be accessed from init-action.ts and passed in to here. That should already have been done on main so you'll just need to do some rerouting.
src/config-utils.test.ts
Outdated
|
|
||
| function setConfigFile(inputFileContents: string, tmpDir: string) { | ||
| fs.writeFileSync(path.join(tmpDir, 'input'), inputFileContents, 'utf8'); | ||
| setInput('config-file', 'input'); |
There was a problem hiding this comment.
I think this setInput function has been removed on main. You now pass the value to initConfig as an ordinary function parameter. Unfortunately this removes a lot of the usefullness of this helper function, so you may unfortunately be best off reverting it.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
LGTM
We'll test it in person later on
| githubUrl: string, | ||
| logger: Logger) { | ||
|
|
||
| queriesInput = queriesInput.trim(); |
There was a problem hiding this comment.
Could be slightly better to avoid doing the trimming in multiple places. Could do it just one before passing to addQueriesFromWorkflow and shouldAddConfigFileQueries
There was a problem hiding this comment.
I've just written some code to do this - I did the trimming in Config.initConfig since that's the best "common ancestor" of the code flows which end up in these functions, so we only need to trim once.
However, I've not pushed it yet as I'm a bit unsure about it - keeping trimming in these functions themselves means that we won't have any odd bugs if they're ever called from anywhere else. It feels a bit risky to have the necessary trimming so far from the code that requires it to have happened.
What do you think about leaving this as it is?
There was a problem hiding this comment.
Robert and I have discussed and we're going to leave this as-is.
Extends the functionality added in #127 so that you can use queries from a workflow and a config file together, if you wish.
@hubwriter FYI, and could you also check the wording in
init/action.ymland the README please?Merge / deployment checklist