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

Issue 11 #78

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

Issue 11 #78

wants to merge 55 commits into from

Conversation

joelreymont
Copy link

No description provided.

self.DestPort = config.Destination.Port
self.Protocol = config.Destination.Protocol
self.Hostname = config.Hostname
self.RefreshInterval = config.RefreshInterval

Choose a reason for hiding this comment

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

I added a logging statement into main that prints out config.RefreshInterval and get:

2015-06-30 21:37:05 ERROR  remote_syslog.go:23 RefreshInterval: 30ns

Adding another logging statement into SetYAML suggests it never gets called. I think we're using a slightly different version of the YAML lib, but in case it's useful, this comment is present in the current src.

Choose a reason for hiding this comment

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

This was the config:

files:
  - /home/leon/test.log
destination:
  host: HOST.papertrailapp.com
  port: PORT
  protocol: tls
new_file_check_interval: 30

Copy link
Author

Choose a reason for hiding this comment

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

I will expand the config test to include checking loaded values. It only ensures the CLI overrides are working correctly now.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Choose a reason for hiding this comment

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

Thanks!

}

func (self *Config) override() error {
configfile := pflag.StringP("configfile", "c", DEFAULT_CONFIG_FILE, "Path to config")

Choose a reason for hiding this comment

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

Heh, this is an interesting one. The configfile switch won't do anything because we've already loaded the config.

Copy link
Author

Choose a reason for hiding this comment

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

Useful later, perhaps, when we support sending the -HUP signal?

On Wed, Jul 1, 2015 at 10:42 PM Leon Sodhi notifications@github.com wrote:

In config.go
#78 (comment)
:

  • if err = yaml.Unmarshal(file, &config); err != nil {
  •   return fmt.Errorf("Could not parse the config file: %s", err)
    
  • }
  • self.Files = config.Files
  • self.DestHost = config.Destination.Host
  • self.DestPort = config.Destination.Port
  • self.Protocol = config.Destination.Protocol
  • self.Hostname = config.Hostname
  • self.RefreshInterval = config.RefreshInterval
  • self.ExcludeFiles = config.ExcludeFiles
  • self.ExcludePatterns = config.ExcludePatterns
  • return nil
    +}

+func (self *Config) override() error {

  • configfile := pflag.StringP("configfile", "c", DEFAULT_CONFIG_FILE, "Path to config")

Heh, this is an interesting one. The configfile switch won't do anything
because we've already loaded the config.


Reply to this email directly or view it on GitHub
https://github.com/papertrail/remote_syslog2/pull/78/files#r33722036.

Choose a reason for hiding this comment

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

This is definitely something we'll have to fix before merging. Many customers have manually specified a config file location in production environments, particularly when running multiple instances of r_s2 with different hostnames. We also use it in our example init.d script.

@joelreymont
Copy link
Author

Oh, yeah, this resolves issue #11

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