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.

Sign up for GitHub

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

Jump to bottom

fix(journald source): Fix units/include_units switch #2567

Merged
merged 4 commits into from
May 13, 2020
Merged

fix(journald source): Fix units/include_units switch #2567

merged 4 commits into from
May 13, 2020

Conversation

ktff
Copy link
Contributor

@ktff ktff commented May 10, 2020

Fixes the bug of getting

May 08 11:05:28.428 WARN vector::sources::journald: The units setting is deprecated, use include_units instead

and using units even if include_units is used.

ref #2558.

@ktff
Switch bool values
033148a 
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@ktff ktff added the source: journald Anything `journald` source related label May 10, 2020
@ktff ktff requested a review from bruceg May 10, 2020 07:53
@ktff ktff self-assigned this May 10, 2020
@ktff ktff changed the title Switch bool values fix(journald source): Fix units/include_units switch May 10, 2020
ktff added 2 commits May 10, 2020 10:22
@ktff
Cover all cases
46f9280 
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@ktff
Remove whitespace
4d9838e 
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
bruceg
bruceg requested changes May 11, 2020
View reviewed changes
src/sources/journald.rs Outdated Show resolved Hide resolved
src/sources/journald.rs Outdated Show resolved Hide resolved
@bruceg
Copy link
Member

bruceg commented May 11, 2020

After seeing this logic mistake I left in, I wonder if it wouldn't be better to match directly on (self.units, self.include_units) so the arms could contain (Some(_), None) for example. It would require four matches instead of three and extra Some-wrapping, but the logic would be much more obvious. That's not necessary for this PR to pass.

@ktff
Copy link
Contributor Author

ktff commented May 12, 2020 •
edited
Loading

That's true, it would be much clearer. I'll change it to that. Unfortunately they are Vec, so it would require some extra code on which we would lose much of the clarity, instead !vec.is_empty() should be fine.

@ktff
Clarify
005c9ff 
Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
@ktff ktff requested a review from bruceg May 12, 2020 10:34
bruceg
bruceg approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

LGTM

@ktff ktff merged commit 2c8a818 into master May 13, 2020
@ktff ktff deleted the ktff/journald_empty_check branch May 13, 2020 06:35
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
@ktff
fix(journald source): Fix units/include_units switch (vectordotde…
b64d817 
…v#2567)

* Switch bool values

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Cover all cases

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Remove whitespace

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>

* Clarify

Signed-off-by: Kruno Tomola Fabro <krunotf@gmail.com>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruceg bruceg bruceg approved these changes

Assignees

@ktff ktff

Labels
source: journald Anything `journald` source related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ktff @bruceg