Skip to content

config: fix Director -> Director resource#2259

Merged
BareosBot merged 5 commits intobareos:masterfrom
florian-at-bareos:dev/fburger/master/fix-queryfile-config
May 13, 2025
Merged

config: fix Director -> Director resource#2259
BareosBot merged 5 commits intobareos:masterfrom
florian-at-bareos:dev/fburger/master/fix-queryfile-config

Conversation

@florian-at-bareos
Copy link
Contributor

@florian-at-bareos florian-at-bareos commented Apr 24, 2025 •
edited
Loading

Fixes #2228: bareos-dir crash when required QueryFile is missing in configuration

This PR fixes the following bugs:

  • Director -> Director resources are not validated (in particular checked for missing 'required' directives)

This PR changes the following behaviours:

  • The QueryFile(Director -> Director) is no longer marked as 'required'). This is necessary in order for the above bug-fix to not crash existing configurations of the Director.
  • Error out of QueryCmd if query file is not specified.

Note:
All other 'required' directives in Director -> Director are "Name" and "Password", if they are missing currently the daemon instantly errors out or crashes. This means that existing configurations must already have these directives and this PR does not invalidate working configurations

Thank you for contributing to the Bareos Project!

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
  • Required backport PRs have been created
  • Correct milestone is set
  • Branch PR-2259 in CD repository was merged into master branch
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR

@florian-at-bareos florian-at-bareos added this to the 25.0.0 milestone Apr 24, 2025
@florian-at-bareos florian-at-bareos self-assigned this Apr 24, 2025
@florian-at-bareos florian-at-bareos changed the title config: validate Director -> Director resource config: fix Director -> Director resource Apr 24, 2025
@florian-at-bareos florian-at-bareos marked this pull request as ready for review April 24, 2025 11:47
bruno-at-bareos
bruno-at-bareos reviewed Apr 24, 2025
View reviewed changes
bruno-at-bareos
bruno-at-bareos reviewed Apr 28, 2025
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Tested seems to work well.
We should have a test in which we didn't defined the queryfile and check if query emit its warning about not able to read the default.
But minor.
could you verify why core/src/dird/ua_query.cc would need a new copyright without any other changes?

@bruno-at-bareos
Copy link
Contributor

bruno-at-bareos commented Apr 29, 2025

I'm still believing that parameter should have its default documented like scriptdir
see
image

@florian-at-bareos
Copy link
Contributor Author

florian-at-bareos commented May 2, 2025

I'm still believing that parameter should have its default documented like scriptdir see image

Yes! forgot to update that schema parameter

bruno-at-bareos
bruno-at-bareos requested changes May 5, 2025
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal:
I've added a fixup commit about the json config, we should use always system-wide installation for those.
From my side I would start to squash some commits and fix latest pr-check warnings

 ✗  Commit checks failed:
        211c1a196 fixup: add normal real path for default_values: headline starts with 'fixup'
        d7e1353c5 dird: remove 'required' from QueryFile directive: body contains line longer 72 chars

@florian-at-bareos florian-at-bareos force-pushed the dev/fburger/master/fix-queryfile-config branch from 211c1a1 to 0761975 Compare May 7, 2025 07:07
bruno-at-bareos
bruno-at-bareos approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Beside the remark on the commit, we're green for merge.

@florian-at-bareos florian-at-bareos force-pushed the dev/fburger/master/fix-queryfile-config branch from 0761975 to e8f726b Compare May 8, 2025 14:18
arogge
arogge reviewed May 12, 2025
View reviewed changes
@florian-at-bareos florian-at-bareos force-pushed the dev/fburger/master/fix-queryfile-config branch from e30f26c to e196e7c Compare May 13, 2025 11:33
@arogge arogge mentioned this pull request May 13, 2025
Merged
7 tasks
florian-at-bareos and others added 5 commits May 13, 2025 16:28
@florian-at-bareos @BareosBot
config: add Director -> Director validation
9bdef93 
as for every other resource
@florian-at-bareos @BareosBot
dird: update QueryFile directive
8dcf5f2 
remove 'required' flag
add default value, description and 'platform specific' flag

This is necessary to not break existing director configurations.
@bruno-at-bareos @BareosBot
docs: config-schema.json fixes
da1e431 
use build path in `autogenerated/bareos-*config-schema.json` files

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@florian-at-bareos @BareosBot
dird_conf: get rid of intermediate string creation
a3debd5
@BareosBot
Update CHANGELOG.md
ec03720
@BareosBot BareosBot force-pushed the dev/fburger/master/fix-queryfile-config branch from 72e7f93 to ec03720 Compare May 13, 2025 16:28
@BareosBot BareosBot merged commit 3845512 into bareos:master May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joergsteffens joergsteffens joergsteffens left review comments

@arogge arogge arogge left review comments

@bruno-at-bareos bruno-at-bareos bruno-at-bareos approved these changes

Assignees

@florian-at-bareos florian-at-bareos

Labels

bug This addresses a bug requires backport to 24

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

bareos-dir crash when required QueryFile is missing in configuration

5 participants

@florian-at-bareos @bruno-at-bareos @joergsteffens @arogge @BareosBot