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

feat: add logging #117

Merged
merged 10 commits into from
Jan 5, 2022
Merged

feat: add logging #117

merged 10 commits into from
Jan 5, 2022

Conversation

kassianh
Copy link
Contributor

@kassianh kassianh commented Dec 28, 2021

https://app.shortcut.com/mineiros/story/4433/add-logging-support-to-terramate


Logging is done for any action or decision.

  • Imperative statements are used for logging actions, simple statements for decisions
  • Logging is done immediately before the action occurs
  • Logging is done AFTER the decision has been made
    • Eg: In an if statement make log directly after entering saying what has happened
    • Simple statement - non-imperative. Eg: 'Changed' flag was set
  • Not all fields are required
  • Format is:
    • level - trace, debug
    • time - Unix or RC3339
    • stack (optional) - Path
    • action- Name of function where the action is found. With () to improve readibility
    • fileContext (optional) - Absolute path of file if reading a file
    • configFile (optional) - Config file associated with action
    • message - Imperative/simple statement describing what is about to happen/has happened.. Period . at end to improve readibility.
  • Things not to log:
    • return statements
    • errrors (are handled differently)

Other relevant info should also be logged

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2021

Codecov Report

Merging #117 (403b115) into main (b93c999) will decrease coverage by 0.90%.
The diff coverage is 30.00%.

❗ Current head 403b115 differs from pull request most recent head e49517e. Consider uploading reports for the commit e49517e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #117      +/-   ##
==========================================
- Coverage   72.33%   71.43%   -0.91%     
==========================================
  Files          24       24              
  Lines        2668     2727      +59     
==========================================
+ Hits         1930     1948      +18     
- Misses        556      593      +37     
- Partials      182      186       +4     
Flag Coverage Δ
tests 71.43% <30.00%> (-0.91%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/terramate/cli/cli.go 61.03% <30.00%> (-3.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b93c999...e49517e. Read the comment docs.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #4433: Add logging support to terramate.

@kassianh
Copy link
Contributor Author

kassianh commented Jan 2, 2022

@mariux Could you have a look at the last commit and check if this is what you're thinking? Also check the comment at the top for assumptions I've made and how I've chosen to structure the logs. Thank you.

cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved
@katcipis katcipis requested a review from mariux January 3, 2022 20:17
hack/mod-check Outdated Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
stack/loader.go Outdated Show resolved Hide resolved
@mariux
Copy link
Contributor

mariux commented Jan 4, 2022

let's get this merged asap as conflicts will always show up...

@mariux mariux marked this pull request as ready for review January 5, 2022 16:59
@mariux mariux requested a review from katcipis January 5, 2022 16:59
@katcipis katcipis requested review from mariux and i4ki January 5, 2022 16:59
Copy link
Contributor

@mariux mariux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@katcipis katcipis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it seems to not break anything + tested manually and default is still silent + activated logging and seems OK (although default is json, on the help it says it is text, not sure, can be fized on another PR).

There is a lot of log entries setting a field "stack" to what is actually the working dir, even on contexts where it is not about stacks at all, and on context where stacks exist the path is incorrect (sometimes using the working dir). But all that can be fixed on a follow up PR.

Thanks for all the work @kassianh 💯

cmd/terramate/cli/cli.go Show resolved Hide resolved
@@ -200,21 +220,34 @@ func newCLI(
}
}

logger := log.With().
Str("action", "newCli()").
Str("stack", wd).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Str("stack", wd).
Str("workingDir", wd).

cmd/terramate/cli/cli.go Show resolved Hide resolved
cmd/terramate/cli/cli.go Show resolved Hide resolved
cmd/terramate/cli/cli.go Show resolved Hide resolved
git/git.go Show resolved Hide resolved
git/git.go Show resolved Hide resolved
git/git.go Show resolved Hide resolved
git/git.go Show resolved Hide resolved
project/project.go Show resolved Hide resolved
@mariux
Copy link
Contributor

mariux commented Jan 5, 2022

@kassianh please ignore requested changes and merge first and open a follow-up PR with all changes addressed.. this way we can reduce need for rebases and can start using it... great inital work! kudos! and big thanks..

@kassianh kassianh merged commit 056ac11 into main Jan 5, 2022
@kassianh kassianh deleted the kassianh/feat/logging branch January 5, 2022 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants