Skip to content

Unwrap errors#404

Merged
olegsu merged 2 commits into
elastic:mainfrom
olegsu:unwrap-errors
Sep 21, 2022
Merged

Unwrap errors#404
olegsu merged 2 commits into
elastic:mainfrom
olegsu:unwrap-errors

Conversation

@olegsu
Copy link
Copy Markdown
Collaborator

@olegsu olegsu commented Sep 20, 2022

Related to #388

This PR includes also:

  1. Update all the logger calls where the regular go vet would report
  2. Update the CI script golangci-lint configuration to run custom print.funcs

Why update vet with print.funcs?

Normally go vet runs on a lot of functions signatures (the full list go tool vet help printf), but not the one that in logp
To overcome this go vet allow to add additional functions to check.
By setting -printfuncs="Infof,Debugf,Warnf,Errorf,Fatalf,Panicf,DPanicf" we can make sure that all the logger calls are covered

Run locally

Output

When having formatting issues, for example l.log.Infof("Waiting for initial reconfiguration from Fleet server... %s")
You would see
image

Additional Reading

@olegsu olegsu requested review from a team as code owners September 20, 2022 12:33
@mergify mergify Bot assigned olegsu Sep 20, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Sep 20, 2022

This pull request does not have a backport label. Could you fix it @olegsu? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@olegsu olegsu linked an issue Sep 20, 2022 that may be closed by this pull request
9 tasks
@oren-zohar oren-zohar self-requested a review September 20, 2022 12:48
Copy link
Copy Markdown
Collaborator

@oren-zohar oren-zohar left a comment

Choose a reason for hiding this comment

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

Love the use of go vet printfuncs

Comment thread .github/workflows/cloudbeat-ci.yml Outdated
- name: Run Tests
id: run_tests
run: |
just run-golang-vet
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💯

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should probably be part of unit-test.yml so we'll run it only once

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I see we are using golangci-lint. Will update its config

@github-actions
Copy link
Copy Markdown

Comment thread .github/workflows/cloudbeat-ci.yml Outdated
- name: Run Tests
id: run_tests
run: |
just run-golang-vet
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

should probably be part of unit-test.yml so we'll run it only once

@olegsu olegsu force-pushed the unwrap-errors branch 9 times, most recently from 72df493 to 717ce96 Compare September 20, 2022 16:23
@olegsu olegsu enabled auto-merge (squash) September 20, 2022 16:23
Copy link
Copy Markdown
Collaborator

@oren-zohar oren-zohar left a comment

Choose a reason for hiding this comment

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

looks great, can you please share a screenshot of what the logs looks like for one of the edited logs?

Comment thread .golangci.yaml Outdated
- Fatalf
- Panicf
- DPanicf
disable-all: true
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not include them if we are passing all of the analyzers? (Tested locally and appears to pass)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Missed this one

@olegsu olegsu force-pushed the unwrap-errors branch 7 times, most recently from 0afa31f to 2048f5f Compare September 21, 2022 08:34
@olegsu olegsu merged commit 7bf5a86 into elastic:main Sep 21, 2022
yashtewari pushed a commit to yashtewari/cloudbeat that referenced this pull request Sep 21, 2022
* fix: error unwrap

* feat: run vet in ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consolidate AWS wrong credentials with other beats

2 participants