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

chore(agw): s1ap_utils.py is cleaned up #13854

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

mpfirrmann
Copy link
Contributor

@mpfirrmann mpfirrmann commented Sep 6, 2022

Summary

This is a clean-up PR for the s1ap_utils.py. After extensive work in #13853, we shuffled some functions around for better visibility and addressed several pylint remarks.

Test Plan

  • The LTE integration tests were run on my fork with GitHub actions with a systemd-base AGW; LINK.

  • Tests ran green for the containerized AGW as well.

Additional Information

  • This change is backwards-breaking

@mpfirrmann mpfirrmann added the DONOTLAND Please don't merge this label Sep 6, 2022
@mpfirrmann mpfirrmann requested review from a team and rsarwad September 6, 2022 12:04
@pull-request-size pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label Sep 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

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: agw Access gateway-related issue label Sep 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Oops! Looks like you failed the Python Format Check.

Howto

♻️ Updated: ✅ The check is passing the Python Format Check after the last commit.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

feg-workflow

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

Results for commit cc28655.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

dp-workflow

14 tests   14 ✔️  2m 10s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit cc28655.

♻️ This comment has been updated with latest results.

@mpfirrmann mpfirrmann linked an issue Sep 6, 2022 that may be closed by this pull request
10 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

agw-workflow

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

Results for commit cc28655.

♻️ This comment has been updated with latest results.

@pull-request-size pull-request-size bot added size/XL Denotes a Pull Request that changes 500-999 lines. and removed size/L Denotes a Pull Request that changes 100-499 lines. labels Sep 12, 2022
Copy link
Contributor

@voisey voisey left a comment

Choose a reason for hiding this comment

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

How was pylint run on this PR? Was it according to some configuration in magma or using default settings?

@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/XL Denotes a Pull Request that changes 500-999 lines. labels Sep 15, 2022
@mpfirrmann mpfirrmann removed the DONOTLAND Please don't merge this label Sep 15, 2022
@mpfirrmann
Copy link
Contributor Author

How was pylint run on this PR? Was it according to some configuration in magma or using default settings?

Default settings. The main motivation for this PR was the reordering of the functions and after that it was more a Boy's Scout thing to clean up. We just wanted to separate these changes from the feature PR.

@voisey
Copy link
Contributor

voisey commented Sep 19, 2022

@mpfirrmann Have you checked the S1AP tests still run after these changes?

@mpfirrmann mpfirrmann changed the title chore(agw): s1ap_utils is cleaned up chore(agw): s1ap_utils.py is cleaned up Sep 20, 2022
@mpfirrmann
Copy link
Contributor Author

@mpfirrmann Have you checked the S1AP tests still run after these changes?

Yes. I edited the PR description

@mpfirrmann mpfirrmann closed this Sep 20, 2022
@mpfirrmann mpfirrmann reopened this Sep 20, 2022
@mpfirrmann mpfirrmann self-assigned this Sep 20, 2022
@mpfirrmann mpfirrmann removed a link to an issue Sep 21, 2022
10 tasks
@mpfirrmann
Copy link
Contributor Author

@rsarwad, do you maybe have some time to take a look at this PR?

num_ul_flows,
dl_flow_rules=None,
ipv6_non_nat=False,
):
Copy link
Contributor

Choose a reason for hiding this comment

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

At line 626, can we use curly brackets to get the values?

Copy link
Contributor

@rsarwad rsarwad left a comment

Choose a reason for hiding this comment

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

Had minor query. Rest of the changes look good to me

Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>
Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>
@mpfirrmann mpfirrmann added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Sep 23, 2022
@crasu crasu merged commit 35fbec2 into magma:master Sep 23, 2022
@mpfirrmann mpfirrmann deleted the pr/cleanup_s1ap_utils branch September 23, 2022 14:09
pruthvihebbani pushed a commit to pruthvihebbani/magma that referenced this pull request Oct 10, 2022
* chore(agw): functions are reordered for better readability
* chore(agw): s1ap_utils is cleaned up for readability

Signed-off-by: Marco Pfirrmann <marco.pfirrmann@tngtech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: agw Access gateway-related issue ready2merge This PR is ready to be merged (is approved and passes all required checks) 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

4 participants