-
Notifications
You must be signed in to change notification settings - Fork 305
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
Allow "additive" queries in workflow by prefixing with "+" #165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -24,6 +24,11 @@ function setInput(name: string, value: string | undefined) { | |||
} | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We'll test it in person later on
@@ -553,6 +549,10 @@ async function addQueriesFromWorkflow( | |||
githubUrl: string, | |||
logger: Logger) { | |||
|
|||
queriesInput = queriesInput.trim(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.yml
and the README please?Merge / deployment checklist