Visitar URL original
Ximalas syslog facility by Ximalas · Pull Request #115 · zrepl/zrepl · GitHub
Skip to content

Ximalas syslog facility - #115

Merged
problame merged 2 commits into
zrepl:masterfrom
Ximalas:Ximalas-syslog-facility
Mar 15, 2019
Merged

problame merged 2 commits into
zrepl:masterfrom
Ximalas:Ximalas-syslog-facility

Conversation

@Ximalas

@Ximalas Ximalas commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

Here's my contribution to issue #60, adding configurable syslog facility to zrepl.

@problame

problame commented Feb 2, 2019

Copy link
Copy Markdown
Member

Thanks for your contribution!
I will merge this PR prior to 0.1, but it'll take 1-2 weeks until I have time for zrepl again.

@Ximalas Ximalas left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's an open question whether to map the string value from the config file to the corresponding syslog.Priority value in parseSyslogOutlet() or in WriteEntry().

Comment thread daemon/logging/logging_outlets.go Outdated
case "local5": fac = syslog.LOG_LOCAL5
case "local6": fac = syslog.LOG_LOCAL6
case "local7": fac = syslog.LOG_LOCAL7
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be done in config.go parseSyslogOutlet, the SyslogOutlet.Facility member should be syslog.Priority ( https://golang.org/pkg/log/syslog/#Priority )

@Ximalas

Ximalas commented Feb 4, 2019 •

Copy link
Copy Markdown
Contributor Author

I'm not sure if I understood you correctly. I can create func ParseSyslogFacility(s string) (p syslog.Priority) in config/config.go and move the mapping there, if that's what you had in mind.

@problame problame left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for following up so quickly!

Now the last bit that is missing is some unit test coverage (see config/*_test.go): I would like to see a check that the default value is indeed syslog.LOG_LOCAL0 and another check that asserts that facility can be overridden by specifying it in the yaml.

FunctionTestRPC in confg_rpc_test.go might serve as as inspiration, although I don't expect all facility names to be tested.

Comment thread config/config_global_test.go Outdated
- type: syslog
level: info
format: human
facility: kern

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please use go string templating ( https://golang.org/pkg/text/template/ ) or similiar to avoid the repetition here

Comment thread daemon/logging/build_logging.go Outdated
case "local7":
out.Facility = syslog.LOG_LOCAL7
default:
out.Facility = syslog.LOG_LOCAL0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be fmt.Errorf("unknown facility name %q", in.Facility)

Comment thread config/config.go Outdated

type SyslogLoggingOutlet struct {
LoggingOutletCommon `yaml:",inline"`
Facility string `yaml:"facility,default=local0"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm, sorry, maybe I was unclear: I would like to see the syslog.Facility here. that should be possible by defining a wrapper type and implementing UnmarshalYAML for it.

@Ximalas

Ximalas commented Feb 5, 2019

Copy link
Copy Markdown
Contributor Author

I'm in unknown waters, that's for sure. I'm stuck at trying to convert strings to syslog.Priority values. All I get from Travis CI is:

--- FAIL: TestOutletTypes (0.00s)
	Error Trace:	config_test.go:42
			config_global_test.go:27
			config_global_test.go:31
	Error:		Received unexpected error yaml: unmarshal errors:
        	  line 1: cannot unmarshal !!str `local0` into syslog.Priority
        
--- FAIL: TestSyslogLoggingOutletFacility (0.00s)
	Error Trace:	config_test.go:42
			config_global_test.go:27
			config_global_test.go:100
	Error:		Received unexpected error yaml: unmarshal errors:
        	  line 7: cannot unmarshal !!str `kern` into syslog.Priority

I had this wonderful idea about creating type SyslogFacilityEnum struct, type SyslogFacility struct, and func (t *SyslogFacilityEnum) UnmarshalYAML(u func(interface{}, bool) error) (err error), but to no avail. I'm clearly missing something.

@problame

problame commented Mar 15, 2019 •

Copy link
Copy Markdown
Member

I fixed the config parsing errors in a private branch.
Please give me write permissions on your PR branch, so that we can continue work within this PR: https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

edit: nvmd, using the http remote worked (see force push)

@problame
problame force-pushed the Ximalas-syslog-facility branch from 69da2be to 9b1a216 Compare March 15, 2019 16:50
@problame
problame force-pushed the Ximalas-syslog-facility branch 2 times, most recently from 6b11f40 to 0de283b Compare March 15, 2019 17:10
@problame
problame force-pushed the Ximalas-syslog-facility branch from 0de283b to a0f301d Compare March 15, 2019 17:18
@problame
problame dismissed their stale review March 15, 2019 17:19

fixed stuff myself

@problame
problame merged commit a0f301d into zrepl:master Mar 15, 2019
@Ximalas

Ximalas commented Mar 15, 2019 via email

Copy link
Copy Markdown
Contributor Author

@problame

Copy link
Copy Markdown
Member

No more action required, except for testing the binary build if possible:
https://circleci.com/gh/zrepl/zrepl/58#artifacts/containers/0
(just click to /go/src/github.com/zrepl/zrepl/artifacts/release )

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.

2 participants