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 start command detection for spdlog #1530

Merged
merged 1 commit into from
Apr 7, 2021
Merged

Fix start command detection for spdlog #1530

merged 1 commit into from
Apr 7, 2021

Conversation

dominiklohmann
Copy link
Member

📔 Description

When configuring spdlog we do different things for client and server. We distinguish between the two by checking whether the current subcommand we're running is start.

However, this returned a false positive for a plugin that had a command <plugin> start. This commit changes to checking whether the full name of the command we're running is start.

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

n/t

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Apr 7, 2021
@dominiklohmann dominiklohmann requested a review from a team April 7, 2021 07:55
@dominiklohmann dominiklohmann enabled auto-merge April 7, 2021 07:58
@dominiklohmann dominiklohmann force-pushed the topic/fix-spdlog-setup branch from df4c198 to 9cac91f Compare April 7, 2021 08:00
tobim
tobim approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

My understanding of invocation allows me to confirm that this change is correct without testing it locally.

lava
lava approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Looks good, I didn't test it myself but I assume you have already checked that the full_name is just start and not vast start in the server case?

@dominiklohmann
Fix start command detection for spdlog
807f5de 
When configuring spdlog we do different things for client and server. We
distinguish between the two by checking whether the current subcommand
we're running is `start`.

However, this returned a false positive for a plugin that had a command
`<plugin> start`. This commit changes to checking whether the full name
of the command we're running is `start`.
@dominiklohmann dominiklohmann force-pushed the topic/fix-spdlog-setup branch from 9cac91f to 807f5de Compare April 7, 2021 08:01
@dominiklohmann
Copy link
Member Author

I assume you have already checked that the full_name is just start and not vast start in the server case?

Yes. I verified that vast start still writes to the vast.db/server.log file.

@dominiklohmann dominiklohmann merged commit 89c00ab into master Apr 7, 2021
@dominiklohmann dominiklohmann deleted the topic/fix-spdlog-setup branch April 7, 2021 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lava lava lava approved these changes

@tobim tobim tobim approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dominiklohmann @lava @tobim