Skip to content
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

fix: log the config name at validation #537

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Conversation

lemeurherve
Copy link
Member

@lemeurherve lemeurherve commented Feb 23, 2022

Fixes #536

Log the config name (which default value is the manifest filename) so when updacli is running on a folder and there is an error it's easier to know which manifest has a problem.

Test

To test this pull request, you can run the following commands:

cp <to_package_directory>
go test

Additional Information

Tradeoff

One additional line per manifest in the output

Potential improvement (and fix of the tradeoff)

This log should even replace the one line 82:

logrus.Infof("Loading Pipeline %q", cfgFile)

WDYT? In that case, this PR could be "fix: log the config name at validation instead of at loading"

So when updacli is running on a folder and there is an error it's easier to know which manifest has a problem.

Fixes updatecli#536
@lemeurherve lemeurherve changed the title fix: log the config name at the beginnning of its validation fix: log the config name at the beginning of its validation Feb 23, 2022
@lemeurherve lemeurherve changed the title fix: log the config name at the beginning of its validation fix: log the config name at validation Feb 23, 2022
@olblak olblak merged commit 5afdcfb into updatecli:main Feb 23, 2022
@lemeurherve
Copy link
Member Author

WDYT? In that case, this PR could be "fix: log the config name at validation instead of at loading"

@olblak I wanted to include it in this PR but waited for your return before doing so. No worry I can do it in another PR, but I'd still like to hear your opinion before :)

IMO it would be better as it would simply replace the current log line done when the files are loaded by one done when each file is validated.

@olblak
Copy link
Member

olblak commented Feb 24, 2022

Sorry that I missed your question, I should have take more care before merging your pullrequest.

IMHO the message from line 82 logrus.Infof("Loading Pipeline %q", cfgFile) contains a different information than the pipeline name so it makes sense to have it. We want to easily identify what went wrong when trying to read the file.

Your PR is about identifying what went wrong when validating the pipeline from the file

For a reason that I ignore the line 105 is duplicated with 114. The line 105 should be removed.

Imho your change should have been a debug and not an info.

thinking loud
I am wondering if it wouldn't be better to notify which pipeline we are in from the engine package

@dduportal dduportal added the bug Something isn't working label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: add the concerned manifest filename in the error output
4 participants