feat(main): log configuration file used #615
Conversation
app/Main.hs
Outdated
| @@ -234,6 +245,7 @@ main = do | |||
| execute CommandOptions {dockerfiles = []} = | |||
| putStrLn "Please provide a Dockerfile" >> exitFailure | |||
| execute cmd = do | |||
| when (showConfigFile cmd) (putStrLn $ getFilePathDescription (configFile cmd)) | |||
There was a problem hiding this comment.
I think this should got to stderr, instead of stdout, so that the parsing of error messages is not disrupted
There was a problem hiding this comment.
sure, this sounds good! could i just get extra clarification on the parsing of error messages and how it's being done right now?
There was a problem hiding this comment.
Oh, that is being done by third parties, like editors and CI systems
There was a problem hiding this comment.
Ah I see. Thank you
|
@lorenzo @seaerchin Most other tools I've used would output this type of information when you pass a |
|
sounds like a good idea! |
|
This branch now needs to be rebased |
|
@lorenzo no worries; could i just ask if the workflow for commits is to rebase or to merge? my main worry is that for rebases, you would have to review the whole PR again, which might not be an issue here because this PR is small but might be an issue for a future PR. also, on a separate note, would you be fine with my suggestion of first creating a verbose option that solely outputs the config file name before adding in more descriptive information? my main reason for doing so is that i don't want scope creep to occur for this PR and due to my unfamiliarity with both haskell and the codebase, prevent any possible errors through limiting the scope |
|
Feel free to merge, if that is easier for you. I really don’t have a suggestion and I don’t mind reviewing again |
|
and yea, I’m ok with the idea of the verbose flag :) |
|
I'll rebase and request another review! But this should be seen as a logging of just the config file rather than as a verbose flag. I'll work on the actual verbose feature and raise it in another follow-up PR! |
|
i think i'll raise the verbose flag as another issue and actually, after thinking through it, we do have a few options. we could do something similar to npm, where we have a log file which we always write to and is enabled to another option is to just have it as what it is right now, and write the information to |
|
We could just always output the config file name to stderr if one was found. I don’t think that would be a problem to anyone |
seaerchin
force-pushed
the
feature/show-config-file
branch
from
d3eba32
to
54963d7
Compare
|
@lorenzo fyi, updated some stuff! would appreciate if you could take a look! |
|
thank you, it looks great! |
Problem
Users want to know what is the configuration file used so that troubleshooting in the event of errors can be made easier
Closes #610
Solution
This PR adds a flag that, when set, displays the configuration file used.
Alternatives Considered
hadolint. this was decided against because this should be an opt in feature as there might be a significant majority of users usinghadolintas is.Manual Tests
hadolintwith the-hoption. verify that the show config option is there.hadolintwith-Vor--verbosebut without a configuration file specified.hadolintshould work but print a message notifying the user that there is no configuration file found.hadolintwith-Vor--verbosewith a valid configuration file specified.hadolintshould work and print the configuration file path.Note: there are currently no unit tests. would appreciate it if someone could point out to me where to add it because
ConfigSpecseems to be on the config file itself and i'm not too sure if it belongs inSpec