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: Raise Error Log when Uplink/Downlink AMBR missing unit #113

Merged
merged 7 commits into from
Aug 16, 2024

Conversation

ntut-xuan
Copy link
Contributor

@ntut-xuan ntut-xuan commented Jul 30, 2024

What's happened?

Hi, I'm Uriah from NYCU Room 215 😄

In this PR, I fix when Uplink/Downlink AMBR in webconsole subscriber page doesn't have unit, it will rasie panic in free5gc SMF.

image

In above image, the Uplink/Downlink AMBR doesn't have unit (Mbps or Kbps), it will make free5gc raise panic in below image.

image

I add error handling in BitRateTokbps function and raise Error in ActivateTunnelAndPDR function when BitRateTokbps function raise the error.

image

Implmentation

  • Add error handling in ActivateTunnelAndPDR

How to review?

  • Check when Uplink/Downlink AMBR doesn't have unit, it should raise error instead of raw panic.

@ianchen0119
Copy link
Contributor

@ntut-xuan

Thanks for your PR.
Please help to fix the ci-linter error.

@andy89923
Please help to review it when you feel free.

@ntut-xuan
Copy link
Contributor Author

ntut-xuan commented Aug 6, 2024

@ntut-xuan

Thanks for your PR. Please help to fix the ci-linter error.

@andy89923 Please help to review it when you feel free.

Hi @ianchen0119, lint issue is fixed. Please approve the workflows.

@andy89923
Copy link
Contributor

Hi @ntut-xuan,

Thanks for the PR!

Overall it's LGTM, but I think we should add Test for the function under util, such as TestBitRateTokbps and TestBitRateTombps.

Could you help with the test function? Or I can provide test function and you help with the testcase.

@ntut-xuan
Copy link
Contributor Author

Hi @ntut-xuan,

Thanks for the PR!

Overall it's LGTM, but I think we should add Test for the function under util, such as TestBitRateTokbps and TestBitRateTombps.

Could you help with the test function? Or I can provide test function and you help with the testcase.

No problem. I'll add it soon.

@ntut-xuan
Copy link
Contributor Author

ntut-xuan commented Aug 7, 2024

Hi @ianchen0119 and @andy89923:

I already add the tests and check the implementation passed lint-chcek.
All testcase follows AAA pattern and it should show what the testcase check clearly. Please review it, thanks!

Since we don't have a folder of tests, I add a folder /internal/util/tests, it should use to contain test file in future.

Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

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

LGTM!
@ianchen0119

@ianchen0119 ianchen0119 merged commit 4967125 into free5gc:main Aug 16, 2024
3 checks passed
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.

3 participants