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

enable custom config file #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kapitainsky
Copy link
Contributor

Sometimes it is useful to use different config files e.g. when different snapshots retentions are needed for different datasets.

I have added "-C, --custom-config </path/to/file>" option

@kapitainsky kapitainsky force-pushed the custom_config branch 2 times, most recently from 390dff4 to dbd66ac Compare April 26, 2023 07:48
@gbytedev
Copy link
Owner

Looks good; I'll try and find some time to test this soon. Note to myself: Document this feature in readme & the default config file itself.

@kapitainsky
Copy link
Contributor Author

Document this feature in readme & the default config file itself.

Both are already included in this PR .

@gbytedev
Copy link
Owner

Both are already included in this PR .

Nope, with 'default config file' I meant default.zfsbud.conf, with documenting in readme, I meant 'introduction' and 'usage' part of the file. Sorry for not being clear on this.

@gbytedev
Copy link
Owner

@kapitainsky Looking at the code, this appears to be overwriting the inline --snapshot-prefix argument which should take precedence in my opinion (haven't tested it though). Maybe we should alter and move the initial snapshot_prefix=$(config_get default_snapshot_prefix) to after the input arguments have been read. If you don't have the time, don't worry, I'll grab it soon when I find some. :)

@kapitainsky
Copy link
Contributor Author

I will look into it. Please note I have been using all these PRs in production for many months now:) And I am very happy with my new functionality

@gbytedev
Copy link
Owner

Please note I have been using all these PRs in production for many months now:) And I am very happy with my new functionality

I'm happy you are and I am thankful for your contribution, but as the maintainer I need to be thinking in more broader terms and cover more use cases. A setting in a configuration file should not be overwriting an inline argument in my opinion.

@kapitainsky
Copy link
Contributor Author

Please note I have been using all these PRs in production for many months now:) And I am very happy with my new functionality

I'm happy you are and I am thankful for your contribution, but as the maintainer I need to be thinking in more broader terms and cover more use cases. A setting in a configuration file should not be overwriting an inline argument in my opinion.

Agree. Good catch. Fixed

Sometimes it is useful to use different config files e.g. when different snapshots retentions are needed for different datasets.

I have added "-C, --custom-config </path/to/file>" option
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.

None yet

2 participants