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

Remove /etc as hardcoded sysconfdir from Nix build #1777

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

dominiklohmann
Copy link
Member

📔 Description

This PR contains two changes:

  • Our static binaries behaved differently than our regular relocatable installations: They were looking for the global configuration in /etc/vast/vast.yaml rather than the expected <prefix>/etc/vast/vast.yaml. This change makes them behave just like regular relocatable installations.
    For users, this means that configuration files at /etc/vast/vast.yaml need to be moved accordingly.
  • We now install an example config file alongside VAST.

📝 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

Read the code, try the static binary from CI.

Our static binaries behaved differently than our regular relocatable
installations: They were looking for the global configuration in
`/etc/vast/vast.yaml` rather than the expected
`<prefix>/etc/vast/vast.yaml`. This change makes them behave just like
regular relocatable installations.

For users, this means that configuration files at `/etc/vast/vast.yaml`
need to be moved accordingly.
@dominiklohmann dominiklohmann added enhancement ✨ bug Incorrect behavior labels Jul 15, 2021
@dominiklohmann dominiklohmann requested a review from a team July 15, 2021 08:28
@mavam
Copy link
Member

mavam commented Jul 15, 2021

Why is the example config all commented out?

@dominiklohmann
Copy link
Member Author

Why is the example config all commented out?

That's intentional: It's an example config file, and if we install it, then we don't want it to mesh with the actual defaults. Unless we find a way to guarantee that defaults and example config are in sync (which we don't right now), I'd rather have everything commented out.

@mavam
Copy link
Member

mavam commented Jul 15, 2021

I would agree if it's an actual .yaml file. But it's an .example file that VAST doesn't look at anyway on startup.

The UX of reading a properly syntax-highlighted file and separate the tuning knobs from the content is not to be dismissed, I think.

@dominiklohmann
Copy link
Member Author

I would agree if it's an actual .yaml file. But it's an .example file that VAST doesn't look at anyway on startup.

Take a look at the changed CMake code in this PR: We install this file to <prefix>/etc/vast/vast.yaml.

@mavam
Copy link
Member

mavam commented Jul 16, 2021

I wonder whether we should simply keep the uncommented example file in the repo and install a transformation (e.g., through sed) at the destination. That would make at least maintaining the example config file easier and avoid mistakes.

Also, avoid running black on vast.yaml.example; the CI was a bit overzealous
here with black thinking the vast.yaml.example was valid Python code.
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

If you can get awk installed in Nix, we're in business in CI.

@dominiklohmann dominiklohmann force-pushed the story/ch26968/nix-sysconfdir branch 2 times, most recently from 9bf1548 to 0de237e Compare July 16, 2021 09:19
@dominiklohmann dominiklohmann merged commit 5304c4c into master Jul 16, 2021
@dominiklohmann dominiklohmann deleted the story/ch26968/nix-sysconfdir branch July 16, 2021 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants