Skip to content

dird: fix odd-even weeks parsing bug in schedule#1210

Merged
pstorz merged 3 commits intobareos:masterfrom
HediBenFraj:dev/HediBenFraj/master/odd-even-weeks-schedule
Aug 16, 2022
Merged

dird: fix odd-even weeks parsing bug in schedule#1210
pstorz merged 3 commits intobareos:masterfrom
HediBenFraj:dev/HediBenFraj/master/odd-even-weeks-schedule

Conversation

@HediBenFraj
Copy link
Contributor

@HediBenFraj HediBenFraj commented Jul 22, 2022 •
edited by bruno-at-bareos
Loading

Description

There is a bug where the config parser is not performing as expected when it comes to scheduling odd/even weeks

config files

Schedule {
  Name = "Odd Weeks"
  Run = w01/w02 sun at 23:10
}

Schedule {
  Name = "Even Weeks"
  Run = w00/w02 fri at 23:10
}

parsed schedule

show schedules
Schedule {
 Name = "Odd Weeks"
 Run = Sun w00,w02,w04,w06,w08,w10,w12,w14,w16,w18,w20,w22,w24,w26,w28,w30,w32,w34,w36,w38,w40,w42,w44,w46,w48,w50,w52 at 23:10
}

Schedule {
 Name = "Even Weeks"
 Run = Fri w01,w03,w05,w07,w09,w11,w13,w15,w17,w19,w21,w23,w25,w27,w29,w31,w33,w35,w37,w39,w41,w43,w45,w47,w49,w51 at 23:10
}

the odd and even weeks are flipped

  • 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
  • [ ] If backport: add original PR number and target branch at top of this file: Backport of PR#000 to bareos-2x
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 test is required (if not, then remove this paragraph)
  • The choice of the type of test (unit test or systemtest) is reasonable
  • Testname matches exactly what is being tested
  • On a fail, output of the test leads quickly to the origin of the fault

@HediBenFraj HediBenFraj changed the title dird: fix odd-even weeks bug in schedule dird: fix odd-even weeks parsing bug in schedule Jul 22, 2022
pstorz
pstorz requested changes Jul 25, 2022
View reviewed changes
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

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

Good that you found the problem :However, the test you added does not check anything.

The test needs to really test that the functionality being tested works as expected.

@pstorz pstorz self-assigned this Jul 25, 2022
@pstorz pstorz marked this pull request as draft July 28, 2022 09:46
@HediBenFraj HediBenFraj force-pushed the dev/HediBenFraj/master/odd-even-weeks-schedule branch from 6a99f56 to c65a572 Compare August 2, 2022 09:06
@pstorz pstorz requested a review from bruno-at-bareos August 4, 2022 09:08
@HediBenFraj HediBenFraj force-pushed the dev/HediBenFraj/master/odd-even-weeks-schedule branch from c65a572 to b485e8b Compare August 5, 2022 10:04
@HediBenFraj HediBenFraj marked this pull request as ready for review August 5, 2022 11:51
bruno-at-bareos
bruno-at-bareos requested changes Aug 8, 2022
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.

I let some comment into the code. Basically I think it will be more coherent to use the same schedule name as state into the documentation. This can be applied also to the test name.

About the change in itself, I would prefer that all the code we have for modulo keep its similarity.

@HediBenFraj HediBenFraj force-pushed the dev/HediBenFraj/master/odd-even-weeks-schedule branch 2 times, most recently from 4121194 to 974cde4 Compare August 10, 2022 12:35
@bruno-at-bareos bruno-at-bareos self-requested a review August 10, 2022 12:42
@HediBenFraj HediBenFraj force-pushed the dev/HediBenFraj/master/odd-even-weeks-schedule branch from 974cde4 to 375b569 Compare August 10, 2022 12:48
bruno-at-bareos
bruno-at-bareos approved these changes Aug 10, 2022
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.

I approve this changes, it even fix the 3rd example we have in the documentation.

Schedule {
  Name = "On the 3rd week in a 5-week-cycle"
  Run = w03/w05 at 23:10
}

Before this PR we obtain

Schedule {
  Name = "On the 3rd week in a 5-week-cycle"
  Run = w02,w07,w12,w17,w22,w27,w32,w37,w42,w47,w52 at 23:10
}

With this PR we start on week 3 then 8 (3+5) etc...

Schedule {
  Name = "On the 3rd week in a 5-week-cycle"
  Run = w03,w08,w13,w18,w23,w28,w33,w38,w43,w48,w53 at 23:10
}

@bruno-at-bareos bruno-at-bareos requested a review from pstorz August 10, 2022 14:29
@bruno-at-bareos
Copy link
Contributor

bruno-at-bareos commented Aug 10, 2022

@pstorz when its build and you redone a quick review, I believe we are ok.

pstorz
pstorz requested changes Aug 11, 2022
View reviewed changes
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

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

See comments.

@HediBenFraj HediBenFraj force-pushed the dev/HediBenFraj/master/odd-even-weeks-schedule branch from 375b569 to 6bc39ab Compare August 12, 2022 09:15
@HediBenFraj HediBenFraj requested a review from pstorz August 12, 2022 09:15
pstorz
pstorz requested changes Aug 12, 2022
View reviewed changes
@HediBenFraj HediBenFraj force-pushed the dev/HediBenFraj/master/odd-even-weeks-schedule branch from 6bc39ab to 3e11fc5 Compare August 12, 2022 10:13
@HediBenFraj HediBenFraj requested a review from pstorz August 12, 2022 10:13
bruno-at-bareos
bruno-at-bareos approved these changes Aug 15, 2022
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.

Nice improvements.

pstorz
pstorz approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member

@pstorz pstorz left a comment

Choose a reason for hiding this comment

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

Good work!

@pstorz pstorz merged commit d3aec92 into bareos:master Aug 16, 2022
@pstorz pstorz deleted the dev/HediBenFraj/master/odd-even-weeks-schedule branch August 16, 2022 14:21
@bruno-at-bareos bruno-at-bareos mentioned this pull request Aug 16, 2022
Merged
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pstorz pstorz pstorz approved these changes

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

Assignees

@pstorz pstorz

Labels

requires backport to 21

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HediBenFraj @bruno-at-bareos @pstorz