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

Added short versions for the configuration files #1637

Merged
merged 1 commit into from
May 16, 2016

Conversation

tsg
Copy link
Contributor

@tsg tsg commented May 13, 2016

For now, the short files are named like beatname.short.yml and I kept the
beatname.yml to be the full one. The intention is to switch them around. The
short ones to become the default and the long ones to be something like
beatname.full.yml or similar.

But before doing that I wanted the others in the team to have a look. The
decision of what to include in the short version is fairly arbitrary, I just
did it based on what I thought makes most sense.

I kept the ES and LS outputs in the configuration file. I considered having
only ES for all the beats except Filebeat, but that would complicate the
generation part and I think it's nice to highlight that we support multiple
outputs.

@tsg tsg added review discuss Issue needs further discussion. labels May 13, 2016
@tsg tsg mentioned this pull request May 13, 2016
14 tasks
@tsg
Copy link
Contributor Author

tsg commented May 13, 2016

Part of #1417.

# Some sample encodings:
# plain, utf-8, utf-16be-bom, utf-16be, utf-16le, big5, gb18030, gbk,
# hz-gb-2312, euc-kr, euc-jp, iso-2022-jp, shift-jis, ...
encoding: plain
Copy link
Member

Choose a reason for hiding this comment

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

I would leave out encoding

@ruflin
Copy link
Member

ruflin commented May 13, 2016

LGTM. +1 on moving forward with this. We can still make small adjustments later if needed.

Two things for the naming:

  • In the repository, I would keep the full one without an extension
  • When downloading and starting a beat, it is important to take the short one by default

The above notes unfortunately conflict with each other :-) My suggestion is to not have the short version in the the top level folder, only under _beat or etc. During building the beat.yml is renamed to beat.full.yml and the short and is then beat.yml. The downside is, that this adds complexity. The idea behind is, that everyone who checks out the repo should have the full config available. Probably there is a better way ...

@urso
Copy link

urso commented May 13, 2016

alternative name for short: min?

I think these very big comments make the minified version still to verbose. Let's try to have at most 1 or 2 lines commenting an option. Maybe with second line being a link to the docs (like we include link to doc reference at top of file, but it's so easily overlooked)

@ruflin
Copy link
Member

ruflin commented May 13, 2016

I like the 2 lines suggestion by @urso with the docs.

@tsg
Copy link
Contributor Author

tsg commented May 13, 2016

I did the 2 comment lines suggestion (but not the link to the docs, that would be quite an effort to maintain) and removed some more stuff as suggested by @ruflin. Let's discuss the rest in the meeting.

@tsg
Copy link
Contributor Author

tsg commented May 13, 2016

Why not just name the short one beatname.yml and beatname.full.yml the complete one in both the repository and the packages? There are people executing them from source, the principle I used so far in packaging is that running from source feels pretty much the same as running from the binary tar.gz.

@tsg tsg force-pushed the add_short_config_versions branch from 4d84f97 to a38e468 Compare May 13, 2016 13:29
@tsg
Copy link
Contributor Author

tsg commented May 13, 2016

Added logging to the short version. I prefer doing the file rename in a follow up PR, so this is ready for merging unless more comments show up or the build fails.

@monicasarbu
Copy link
Contributor

LGTM

For now, the short files are named like `beatname.short.yml` and I kept the
`beatname.yml` to be the full one.  The intention is to switch them around. The
short ones to become the default and the long ones to be something like
`beatname.full.yml` or similar.

But before doing that I wanted the others in the team to have a look. The
decision of what to include in the short version is fairly arbitrary, I just
did it based on what I thought makes most sense.

I kept the ES and LS outputs in the configuration file. I considered having
only ES for all the beats except Filebeat, but that would complicate the
generation part and I think it's nice to highlight that we support multiple
outputs.
@tsg tsg force-pushed the add_short_config_versions branch from a38e468 to 9abac74 Compare May 16, 2016 18:48
@monicasarbu monicasarbu merged commit 016ca9a into elastic:master May 16, 2016
@tsg tsg deleted the add_short_config_versions branch August 25, 2016 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants