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

fix(dp): add max_ibw_mhz validation #13293

Merged
merged 4 commits into from Jul 18, 2022

Conversation

hejjestemjarek
Copy link
Contributor

Signed-off-by: Jarosław Jaszczuk jaroslaw.jaszczuk@freedomfi.com

Summary

Added validation for max_ibw_mhz field, that ensures the value is:

  • >= 5
  • <= 150
  • multiple of 5

Test Plan

Added test cases.

Additional Information

  • This change is backwards-breaking

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Jul 14, 2022
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: orc8r Orchestrator-related issue label Jul 14, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2022

feg-workflow

    2 files  203 suites   39s ⏱️
374 tests 374 ✔️ 0 💤 0
388 runs  388 ✔️ 0 💤 0

Results for commit 167efa3.

♻️ This comment has been updated with latest results.

@hejjestemjarek hejjestemjarek force-pushed the validate_max_ibw_mhz_value branch 2 times, most recently from 3595957 to 766d021 Compare July 14, 2022 10:37
@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2022

dp-workflow

18 tests   18 ✔️  4m 21s ⏱️
  2 suites    0 💤
  2 files      0

Results for commit 167efa3.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2022

cloud-workflow

1 000 tests   1 000 ✔️  2m 43s ⏱️
   356 suites         0 💤
       7 files           0

Results for commit 167efa3.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2022

agw-workflow

615 tests   611 ✔️  3m 49s ⏱️
    2 suites      4 💤
    2 files        0

Results for commit 167efa3.

♻️ This comment has been updated with latest results.

@@ -21,6 +21,9 @@ definitions:
description: this is the maximum allowed difference in MHz between leftmost end of leftmost channel and rightmost end of rightmost channel used by a Base Station (eNB)
type: integer
x-nullable: false
maximum: 150
minimum: 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change it to 20, so that is it always greater than bandwidth of a single channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed minimum: 5 from swagger validation, but modified MutableCbsd's ValidateModel func to ensure that max_ibw_mhz is not less than bandwidth_mhz.

}, {
name: "Should validate max ibw mhz not multiple of 5",
data: b.NewMutableCbsdModelPayloadBuilder().WithMaxIbwMhz(7).Payload,
expectedError: "max_ibw_mhz in body should be a multiple of 5",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a test for if m.Capabilities.MaxIbwMhz < m.FrequencyPreferences.BandwidthMhz and also for this !*m.GrantRedundancy && *m.CarrierAggregationEnabled, since I've noticed that it is missing for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added them in a separate test, since original TestMutableCbsd_Validate covers only swagger generated Validate func and would not cover ValidateModel func.

@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Jul 15, 2022
Jarosław Jaszczuk added 4 commits July 18, 2022 14:09
Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>
Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>
Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>
Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>
@hejjestemjarek hejjestemjarek marked this pull request as ready for review July 18, 2022 13:20
@hejjestemjarek hejjestemjarek requested review from a team and m-trojanowski July 18, 2022 13:20
@jkmar jkmar merged commit e0cf136 into magma:master Jul 18, 2022
@hejjestemjarek hejjestemjarek deleted the validate_max_ibw_mhz_value branch July 19, 2022 09:00
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
* fix(dp): add max_ibw_mhz validation

Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>

* validate that max_ibw_mhz is not less than bandwidth_mhz

Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>

* add MutableCbsd.ValidateModel tests

Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>

* reorganize validation_test.go imports

Signed-off-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>

Co-authored-by: Jarosław Jaszczuk <jaroslaw.jaszczuk@freedomfi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: orc8r Orchestrator-related issue size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants