Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Create a separate log package #27

Merged
merged 15 commits into from
Mar 10, 2022
Merged

Conversation

farshidtz
Copy link
Member

@farshidtz farshidtz commented Mar 8, 2022

This PR adds a separate logging package with extended functionality.

It:

Test by adding the following to go.mod:

replace github.com/canonical/edgex-snap-hooks/v2 => github.com/farshidtz/edgex-snap-hooks/v2 da083736dcb18258dd425431d06df0b42e96760c

Add to configure hook:

log.SetComponentName("configure")
log.Infof("info testing new library")
log.Debugf("debug testing new library")
log.Warnf("warn testing new library")
log.Errorf("err testing new library")
log.Errorf("err testing new library 2")
os.Exit(1)

Build and install. Logs should show:

$ sudo snap install --dangerous ./edgex-device-mqtt_2.2.0-dev.9_amd64.snap 
error: cannot perform the following tasks:
- Run configure hook of "edgex-device-mqtt" snap if present (run hook "configure": 
-----
edgex-device-mqtt.configure: err testing new library
edgex-device-mqtt.configure: err testing new library 2
-----)
$ journalctl -f | grep "new library"
Mar 10 10:39:54 farshid-cirrus7 edgex-device-mqtt.configure[59291]: info testing new library
Mar 10 10:39:54 farshid-cirrus7 edgex-device-mqtt.configure[59291]: warn testing new library
Mar 10 10:39:54 farshid-cirrus7 edgex-device-mqtt.configure[59291]: err testing new library
Mar 10 10:39:54 farshid-cirrus7 edgex-device-mqtt.configure[59291]: err testing new library 2
Mar 10 10:39:54 farshid-cirrus7 snapd[1349]: edgex-device-mqtt.configure: err testing new library
Mar 10 10:39:54 farshid-cirrus7 snapd[1349]: edgex-device-mqtt.configure: err testing new library 2

@farshidtz farshidtz marked this pull request as ready for review March 8, 2022 12:23
@farshidtz
Copy link
Member Author

Added more testing instructions.

Copy link
Contributor

@siggiskulason siggiskulason left a comment

Choose a reason for hiding this comment

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

LGTM. Excellent, good to have those variable argument functions instead of needing to create a string.

log/log.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MonicaisHer MonicaisHer left a comment

Choose a reason for hiding this comment

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

@farshidtz Thanks a lot. Please see the inline comments.

@farshidtz
Copy link
Member Author

Thanks for the reviews. I removed the option to turn on debug via environment variable because it was very hard to make it usable since snap options are persisted and override env vars by convention. Allowing both would have just made things very confusing for the user and tests.

@farshidtz farshidtz merged commit 0506159 into canonical:master Mar 10, 2022
@farshidtz farshidtz deleted the log-package branch March 10, 2022 17:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn function logs as syslog error Add logging functions with string formatting Redundant snap name input
3 participants