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

dird: extend the list command to be able to query volumes and pools by ID #1041

Conversation

alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Jan 7, 2022

Description:

This PR extends the list command to give the user the ability to list volumes by ID using list volumeid=xx or list mediaid=xx. Also it give the ability to list pools by Id using list poolid=xx

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)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Separate commit for this PR in the CHANGELOG.md, PR number referenced is same
  • Commit descriptions are understandable and well formatted
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
  • bareos-check-sources --since-merge does not report any problems
  • git status should not report modifications in the source tree after building and testing
Tests
  • Decision taken that a system- or unittest is required (if not, then remove this paragraph)
  • The decision towards a systemtest is reasonable compared to a unittest
  • Testname matches exactly what is being tested
  • Output of the test leads quickly to the origin of the fault

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/extend-list-media-pool branch from 6bc300f to a78b8aa Compare January 11, 2022 14:37
@pstorz pstorz self-assigned this Jan 13, 2022
@pstorz pstorz requested review from franku and fbergkemper and removed request for franku January 13, 2022 11:10
Copy link
Contributor

@franku franku left a comment

Choose a reason for hiding this comment

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

Started a review. Looks good so far, pls read my comments.

core/src/dird/ua_output.cc Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/extend-list-media-pool branch from a78b8aa to 73548f2 Compare January 13, 2022 16:08
@alaaeddineelamri
Copy link
Contributor Author

Started a review. Looks good so far, pls read my comments.

Thanks for the review @franku ! We assigned you by mistake as a reviewer, but it's the good kind of mistake :D

@fbergkemper
Copy link
Contributor

fbergkemper commented Jan 14, 2022

I'd change the PR subject to something like ...

dird: extend the list command to be able to query volumes and pools by ID

... which I think is more precise.

The PR description should be adjusted in that way and also mention the newly introduced arguments poolid, mediaid/volumeid, I feel.

Copy link
Contributor

@fbergkemper fbergkemper left a comment

Choose a reason for hiding this comment

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

I'd split commit e71aa10 into two separate ones.

a) docs: update list command documentation
b) dird: add newly introduced list command arguments to the command line help

Copy link
Contributor

@fbergkemper fbergkemper left a comment

Choose a reason for hiding this comment

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

I'd squash 910394a into bfe6e03 .

Copy link
Contributor

@fbergkemper fbergkemper left a comment

Choose a reason for hiding this comment

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

I think 8a49a2b should be squashed into 3eb67fc .

Copy link
Contributor

@fbergkemper fbergkemper left a comment

Choose a reason for hiding this comment

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

I'd also squash 73548f2 into 3eb67fc

@alaaeddineelamri alaaeddineelamri changed the title dir: Listing volumes and pools using IDs dird: extend the list command to be able to query volumes and pools by ID Jan 17, 2022
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/extend-list-media-pool branch from 73548f2 to a62304a Compare January 17, 2022 09:27
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/extend-list-media-pool branch from a62304a to 6c49b7e Compare January 18, 2022 09:54
CHANGELOG.md Outdated Show resolved Hide resolved
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/extend-list-media-pool branch from 6c49b7e to 9a9d744 Compare January 18, 2022 10:03
Copy link
Contributor

@fbergkemper fbergkemper left a comment

Choose a reason for hiding this comment

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

Regarding commit 9a9d744 :

To be consistent the commit title/subject should be "Update CHANGELOG.md" as we usually do for these types of commits.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/extend-list-media-pool branch from 9a9d744 to 2519df9 Compare January 18, 2022 10:55
@fbergkemper fbergkemper self-requested a review January 18, 2022 11:50
Copy link
Contributor

@fbergkemper fbergkemper 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 to me.

@pstorz pstorz merged commit ed5d88f into bareos:master Jan 19, 2022
@pstorz pstorz deleted the dev/alaaeddineelamri/master/extend-list-media-pool branch January 19, 2022 10:18
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

4 participants