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.

Sign up for GitHub

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

Jump to bottom

[SessionD] AMF to SMF 5G related proto added #2543

Merged
merged 5 commits into from Sep 23, 2020
Merged

[SessionD] AMF to SMF 5G related proto added #2543

merged 5 commits into from Sep 23, 2020

Conversation

sanjay-ACL
Copy link
Member

@sanjay-ACL sanjay-ACL commented Aug 31, 2020 •
edited

Signed-off-by: sanjay-ACL sanjaya.k@altencalsoftlabs.com

Summary

Added 5G specific proto definition.

Test Plan

Tested through other PR and test log can be found in the PR - [SessionD] 5G Session state manager and Enforcer code #2544

Additional Information

  • This change is backwards-breaking

@ulaskozat ulaskozat added product: 5g mvc 5G use-case support component: agw Access gateway-related issue type: enhancement Enhancements to existing features labels Sep 2, 2020
@magma magma deleted a comment from magmabot Sep 17, 2020
themarwhal
themarwhal requested changes Sep 17, 2020
View reviewed changes
lte/protos/session_manager.proto Show resolved Hide resolved
lte/protos/session_manager.proto Outdated Show resolved Hide resolved
@sanjay-ACL sanjay-ACL marked this pull request as ready for review September 17, 2020 11:55
themarwhal
themarwhal reviewed Sep 18, 2020
View reviewed changes
lte/protos/session_manager.proto Show resolved Hide resolved
lte/protos/session_manager.proto Show resolved Hide resolved
lte/protos/session_manager.proto Show resolved Hide resolved
uri200
uri200 reviewed Sep 18, 2020
View reviewed changes
lte/protos/session_manager.proto Show resolved Hide resolved
lte/protos/session_manager.proto Show resolved Hide resolved
lte/protos/session_manager.proto Outdated Show resolved Hide resolved
@themarwhal
Copy link
Member

Also the insync-checkin test is failing because you did not commit the generated file corresponding to it. Please run ./build.py -g in magma/orc8r/cloud/docker and commit the generated changes.

@themarwhal
Copy link
Member

Also in general, if the PR is does not require a test you can specify that in the TestPlan. Otherwise, if it is tested in another PR, please specify which ones.

@sanjay-ACL
Copy link
Member Author

Also in general, if the PR is does not require a test you can specify that in the TestPlan. Otherwise, if it is tested in another PR, please specify which ones.

Yes, updated the testing section of the PR.

themarwhal
themarwhal approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@themarwhal themarwhal left a comment

Choose a reason for hiding this comment

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

Don't have any more immediate feedback on this, so let's just merge this to unblock other development.

lte/protos/session_manager.proto Show resolved Hide resolved
uri200
uri200 reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@uri200 uri200 left a comment •
edited

Choose a reason for hiding this comment

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

by the way, there is a insync error in CI. You will need to regenerate the protos and add the result to fix that. Otherwise it won't let you merge

Just go to ~/magma/feg/gateway/docker and run ./build -g (you will need docker installed). Add the changes to this PR

@sanjay-ACL
Copy link
Member Author

by the way, there is a insync error in CI. You will need to regenerate the protos and add the result to fix that. Otherwise it won't let you merge

Just go to ~/magma/feg/gateway/docker and run ./builde -g (you will need docker installed). Add the changes to this PR

@uri200 and @themarwhal , would you psl. let us know, what are those auto generated files and respective path.

@themarwhal
Copy link
Member

@sanjay-ACL here is the file necessary to be commited: https://app.circleci.com/pipelines/github/magma/magma/3731/workflows/4eba3c4c-e109-4b06-86ca-70857857c579/jobs/38093/parallel-runs/0/steps/0-109

@sanjay-ACL
Copy link
Member Author

@sanjay-ACL here is the file necessary to be commited: https://app.circleci.com/pipelines/github/magma/magma/3731/workflows/4eba3c4c-e109-4b06-86ca-70857857c579/jobs/38093/parallel-runs/0/steps/0-109

Thank you. So lte/cloud/go/protos/session_manager.pb.go has to be committed.
After running your code found as below:

#!/bin/bash -eo pipefail
git status
On branch pull/2543
Changes to be committed:
(use "git reset HEAD ..." to unstage)

modified:   lte/cloud/go/protos/session_manager.pb.go

CircleCI received exit code 0

@sanjay-ACL
Copy link
Member Author

by the way, there is a insync error in CI. You will need to regenerate the protos and add the result to fix that. Otherwise it won't let you merge

Just go to ~/magma/feg/gateway/docker and run ./build -g (you will need docker installed). Add the changes to this PR

Done and committed modified auto-generated file: lte/cloud/go/protos/session_manager.pb.go

@sanjay-ACL
AMF to SMF 5G related proto added
37fc08b 
Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>
@sanjay-ACL
addressed all review comments and added new proto defination for resp…
aa421d7 
…onse message

Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>
@sanjay-ACL
Addressed review comments and cleaned commented code
aeec7b1 
Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>
@sanjay-ACL
Addressed all review comments and refined the proto file
0cf103c 
Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>
@sanjay-ACL
rebased + auto genarated session manager pb.go
40c1f62 
Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>
@themarwhal themarwhal merged commit 2905687 into magma:master Sep 23, 2020
rdefosse pushed a commit to rdefosse/magma that referenced this pull request Oct 9, 2020
@sanjay-ACL @rdefosse
[SessionD] AMF to SMF 5G related proto added (magma#2543)
6ad4adc 
* AMF to SMF 5G related proto added

Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>

* addressed all review comments and added new proto defination for response message

Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>

* Addressed review comments and cleaned commented code

Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>

* Addressed all review comments and refined the proto file

Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>

* rebased + auto genarated session manager pb.go

Signed-off-by: sanjay-ACL <sanjaya.k@altencalsoftlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uri200 uri200 uri200 approved these changes

@sandeepkmall sandeepkmall sandeepkmall left review comments

@themarwhal themarwhal themarwhal approved these changes

@ssanadhya ssanadhya Awaiting requested review from ssanadhya

@ulaskozat ulaskozat Awaiting requested review from ulaskozat

Assignees

@sanjay-ACL sanjay-ACL

Labels
component: agw Access gateway-related issue product: 5g mvc 5G use-case support type: enhancement Enhancements to existing features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sanjay-ACL @themarwhal @uri200 @sandeepkmall @ulaskozat