-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add config files to Bluehawk #147
Add config files to Bluehawk #147
Conversation
- Currently only processes "snip" actions.
- Improve log messages
- More prettifying logs
- Clean up unused imports
@@ -1,4 +1,3 @@ | |||
import { ConsoleActionReporter } from "./actions/ConsoleActionReporter"; |
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 import wasn't used.
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.
Good start... let's discuss
@@ -22,7 +22,7 @@ The frontend of Bluehawk. | |||
|
|||
#### Defined in | |||
|
|||
[src/bluehawk/bluehawk.ts:44](https://github.com/mongodben/Bluehawk/blob/be77c09/src/bluehawk/bluehawk.ts#L44) | |||
[src/bluehawk/bluehawk.ts:42](https://github.com/krollins-mdb/bluehawk/blob/f65f7b1e/src/bluehawk/bluehawk.ts#L42) |
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.
Hm. I don't think the generated docs should have been committed anyway.
` | ||
Bluehawk | ||
` | ||
)}`); |
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.
Probably don't want to execute at file scope - this will print even for library users loading this file
|
||
// Look for "bluehawk.config.yaml" in current working directory. If not | ||
// there, traverse up the file path to system root. | ||
const getRootConfigFile = async ( |
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.
Not sure we even want this functionality?
In what situations would you want to have bluehawk crawl up a directory?
I could definitely see it going down from where you run the command (e.g. run bluehawk in root of repo will execute bluehawk configs in subdirectories).
I fear going up would lead to a lot of unexpected behavior.
rootConfigPath = configFilePath; | ||
|
||
return parseConfigFile(configFilePath); | ||
} else if (directory == "/") { |
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.
Is this portable? /
is root only on UNIX-y systems
|
||
break; | ||
} | ||
} |
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.
We should think about error (exception) handling here - does a throw from one action disrupt all actions?
|
||
break; | ||
|
||
default: |
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 isn't as allowing for extensibility as I would hope - I think a better approach would be to collect all the possible actions somewhere, allow plugins to add to the list of available actions, then use the action list here.
I think such a thing can be done in a subsequent PR.
|
||
// Look for and process config files in child directories | ||
async function processSubconfigFiles() { | ||
const ignores: string[] = [".git"]; |
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.
The cli already exposes some ignore patterns so we could probably use that here
name: command, | ||
}); | ||
|
||
switch (command) { |
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.
We ought to refactor this since it's using more or less the same code to do the same thing but in two places.
}); | ||
|
||
// Run config commands | ||
for (let index = 0; index < subConfig.commands.length; index++) { |
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.
Refactor to something like 'executeConfig' for example
Closing this PR as it's clear that I haven't had time to get this properly implemented. I'll plan time to come back to this in the future. |
Closes #122
This PR adds the ability to define and orchestrate Bluehawk commands using YAML config files. While I was in there, I also took the opportunity to make the terminal messages easier to parse. This was necessary, as it could quickly become unclear which command came from which config file if there are more than one config file.
Potential issue: