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: Enable govet linter and fix reported errors #170

Merged
merged 5 commits into from Jan 14, 2024

Conversation

ghislainbourgeois
Copy link
Contributor

Enables the govet linter and fix reported errors.

@onf-bot
Copy link
Contributor

onf-bot commented Jan 2, 2024

Can one of the admins verify this patch?

1 similar comment
@onf-bot
Copy link
Contributor

onf-bot commented Jan 2, 2024

Can one of the admins verify this patch?

@gab-arrobo
Copy link
Contributor

ok to test

@gab-arrobo
Copy link
Contributor

@ghislainbourgeois, FYI. This branch is out-of-date with the base branch

@ghislainbourgeois
Copy link
Contributor Author

@ghislainbourgeois, FYI. This branch is out-of-date with the base branch

Fixed

gmm/handler.go Outdated Show resolved Hide resolved
gmm/handler.go Outdated Show resolved Hide resolved
context/amf_ue.go Outdated Show resolved Hide resolved
context/amf_ue.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gab-arrobo gab-arrobo Jan 3, 2024

Choose a reason for hiding this comment

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

Why are these changes needed? These changes are not due to enabling govet, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no, the changes made in metrics/kafka.go made this test fail because of a nil pointer. It was working before because the default value was an empty struct and a method could be dispatched, but the default value of a pointer is nil.

@gab-arrobo
Copy link
Contributor

@ghislainbourgeois, did you have a chance to run an end-to-end test using the changes from this PR? I am asking this because I am running a test using AiaB (w/ gnbsim) and the test fails as shown below:

$ make 5g-test
...
...
2024-01-03T17:29:41Z [INFO][GNBSIM][Profile][profile2] ExecuteProfile ended
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Profile Name: profile2 , Profile Type: pdusessest
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Ue's Passed: 0 , Ue's Failed: 5
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Profile Errors:
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007487, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007488, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007489, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007490, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007491, profile timeout
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Profile Status: FAIL
make: *** [Makefile:679: 5g-test] Error 1

@ghislainbourgeois
Copy link
Contributor Author

@ghislainbourgeois, did you have a chance to run an end-to-end test using the changes from this PR? I am asking this because I am running a test using AiaB (w/ gnbsim) and the test fails as shown below:

$ make 5g-test
...
...
2024-01-03T17:29:41Z [INFO][GNBSIM][Profile][profile2] ExecuteProfile ended
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Profile Name: profile2 , Profile Type: pdusessest
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Ue's Passed: 0 , Ue's Failed: 5
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Profile Errors:
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007487, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007488, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007489, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007490, profile timeout
2024-01-03T17:29:41Z [ERRO][GNBSIM][Summary] imsi:imsi-208930100007491, profile timeout
2024-01-03T17:29:41Z [INFO][GNBSIM][Summary] Profile Status: FAIL
make: *** [Makefile:679: 5g-test] Error 1

I did not test with AiaB, I will investigate.

@gab-arrobo
Copy link
Contributor

I did not test with AiaB, I will investigate.

Well, you do not have to use AiaB, but my point was to check whether you had tested the changes using an end-to-end test (e.g., AiaB, OnRamp, etc.)

@ghislainbourgeois
Copy link
Contributor Author

I did not test with AiaB, I will investigate.

Well, you do not have to use AiaB, but my point was to check whether you had tested the changes using an end-to-end test (e.g., AiaB, OnRamp, etc.)

I had not yet no. I am able to reproduce the issue, and it seems to be because the mutex is uninitialized. I will push a fix.

@ghislainbourgeois
Copy link
Contributor Author

I pushed a fix that I was able to confirm working end-to-end with gnbsim.

Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

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

+1 (tested changes with AiaB)

@gab-arrobo gab-arrobo merged commit f794496 into omec-project:master Jan 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants