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(nms): nginx ssl issue #15215

Merged
merged 3 commits into from
Jun 19, 2023
Merged

Conversation

prabinakpattnaik
Copy link
Contributor

Below error is observed when creating orc8r pods:
2023/06/15 13:34:36 [emerg] 1#1: unknown directive "ssl" in /etc/nginx/conf.d/nginx_proxy_ssl.conf:3
nginx: [emerg] unknown directive "ssl" in /etc/nginx/conf.d/nginx_proxy_ssl.conf:3

orc8r nms-nginx-proxy-7454448447-xzc8l 0/1 CrashLoopBackOff 7 (119s ago) 16m

@prabinakpattnaik prabinakpattnaik requested a review from a team as a code owner June 15, 2023 14:29
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Jun 15, 2023
@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: nms NMS-related issue label Jun 15, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 15, 2023

DP Lint & Test

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit b521d2c.

♻️ This comment has been updated with latest results.

Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

Code changes look good.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 15, 2023

NMS Lint & Test

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit b521d2c.

♻️ This comment has been updated with latest results.

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>
@maxhbr
Copy link
Member

maxhbr commented Jun 16, 2023

CI is red, due to out of disk space in a build:

2023-06-16_11-12-31

and nms yarn test is red: https://github.com/magma/magma/actions/runs/5280377687/jobs/9558018580?pr=15215

@prabinakpattnaik
Copy link
Contributor Author

@maxhbr , any idea why "nms yarn test" is failed. Is it lint error or test case failed error.

@maxhbr
Copy link
Member

maxhbr commented Jun 16, 2023

you can look in the log and see the test summary:

Summary of all failing tests
FAIL app/views/traffic/__tests__/TrafficOverviewTest.tsx (8.891 s)
  ● <TrafficDashboard APNs/> › renders

    TypeError: Cannot redefine property: location

      319 |
      320 |   beforeEach((): void => {
    > 321 |     window.location = {
          |     ^
      322 |       pathname: '/nms/test/traffic/apn',
      323 |     } as Location;
      324 |

      at Object.<anonymous> (app/views/traffic/__tests__/TrafficOverviewTest.tsx:321:5)

  ● <TrafficDashboard APNs/> › renders

    TypeError: Cannot redefine property: location

      340 |
      341 |   afterEach((): void => {
    > 342 |     window.location = location;
          |     ^
      343 |   });
      344 |
      345 |   const Wrapper = () => (

      at Object.<anonymous> (app/views/traffic/__tests__/TrafficOverviewTest.tsx:342:5)

  ● <TrafficDashboard APNs/> › shows prompt when remove apn is clicked

    TypeError: Cannot redefine property: location

      319 |
      320 |   beforeEach((): void => {
    > 321 |     window.location = {
          |     ^
      322 |       pathname: '/nms/test/traffic/apn',
      323 |     } as Location;
      324 |

      at Object.<anonymous> (app/views/traffic/__tests__/TrafficOverviewTest.tsx:321:5)

  ● <TrafficDashboard APNs/> › shows prompt when remove apn is clicked

    TypeError: Cannot redefine property: location

      340 |
      341 |   afterEach((): void => {
    > 342 |     window.location = location;
          |     ^
      343 |   });
      344 |
      345 |   const Wrapper = () => (

      at Object.<anonymous> (app/views/traffic/__tests__/TrafficOverviewTest.tsx:342:5)


Test Suites: 1 failed, 4 skipped, 52 passed, 53 of 57 total
Tests:       2 failed, 4 skipped, 349 passed, 355 total

there are test failures

@prabinakpattnaik
Copy link
Contributor Author

Yes, I have seen yesterday on that logs but could not get the clue for whether these errors are link with changes or existing error.

@maxhbr
Copy link
Member

maxhbr commented Jun 16, 2023

it seems to be a general issue that needs to be resolved. Also other PRs have the same issues in the log https://github.com/magma/magma/actions/runs/5247558970/jobs/9477884882?pr=15211

@maxhbr maxhbr mentioned this pull request Jun 16, 2023
1 task
@prabinakpattnaik
Copy link
Contributor Author

@maxhbr , can we do force merge the changes as both CI checks are failing because of one is know issue and another is disk space issue.

@maxhbr
Copy link
Member

maxhbr commented Jun 16, 2023

see https://magmacore.slack.com/archives/C01Q1T14YJ2/p1686916038724709?thread_ts=1686905954.161159&cid=C01Q1T14YJ2, since there is no process for force merges.

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>
@@ -318,6 +318,8 @@ describe('<TrafficDashboard APNs/>', () => {
const networkId = 'test';

beforeEach((): void => {
// @ts-ignore
delete window.location;
Copy link
Member

Choose a reason for hiding this comment

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

if you start working on fixing CI runs, force merges are no longer an option. Would also fit to be put into its own PR?

Copy link
Member

Choose a reason for hiding this comment

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

Hey, can you remove that commit again, so that the impact of a potential force merge is smaller? Also: the commit message is not very helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

Copy link
Member

Choose a reason for hiding this comment

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

ping: can someone drop that commit, so that we can move forward?

@lucasgonze
Copy link
Contributor

Seems like the underlying failure is meaningful:

Warning: A component is changing a controlled input to be uncontrolled. This is likely caused by the value changing from a defined to undefined, which should not happen. Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components

However, the code in this commit doesn't do anything in NMS, so the problem must be a prior commit. Do you agree?

@maxhbr
Copy link
Member

maxhbr commented Jun 16, 2023

see: #15215 (comment)

the NMS error already appeared in other CI runs before this change. So, for NMS the answer is probably yes.

@prabinakpattnaik
Copy link
Contributor Author

I have raised new PR for this issue fix.
15217.

@maxhbr
Copy link
Member

maxhbr commented Jun 19, 2023

Good, that change can be discussed there and the decision whether it is appropriate does not block this PR.

@maxhbr
Copy link
Member

maxhbr commented Jun 19, 2023

@prabinakpattnaik : you still need to drop 34da9f8 from this PR to unblock it.

@prabinakpattnaik
Copy link
Contributor Author

@maxhbr , I am doing now.

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>
@maxhbr
Copy link
Member

maxhbr commented Jun 19, 2023

now it does the change, and reverts it again. But after squashing that should be hidden.

Now we just need to find someone that is able to force-merge. This was made harder with some of the recent policy changes (ping @lucasgonze). see also: https://magmacore.slack.com/archives/C01Q1T14YJ2/p1687165644395929?thread_ts=1686905954.161159&cid=C01Q1T14YJ2

@maxhbr maxhbr merged commit 00fde73 into magma:master Jun 19, 2023
89 of 91 checks passed
lucasgonze pushed a commit to lucasgonze/magma that referenced this pull request Feb 29, 2024
Force merge with go from Lucas Gonze: https://magmacore.slack.com/archives/C01Q1T14YJ2/p1687184855726759

* fix(nms): nginx ssl issue

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>

* fix(nms): nginx ssl issue

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>

* fix(nms): nginx ssl issue

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>

---------

Signed-off-by: prabina pattnaik <prabinak@wavelabs.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: nms NMS-related issue size/XS Denotes a PR that changes 0-9 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants