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

chore(nms): Upgrade chart-js-2 and react-chart-js #13590

Merged
merged 2 commits into from
Sep 9, 2022
Merged

chore(nms): Upgrade chart-js-2 and react-chart-js #13590

merged 2 commits into from
Sep 9, 2022

Conversation

thmsschmitt
Copy link
Contributor

@thmsschmitt thmsschmitt commented Aug 11, 2022 •
edited by Neudrino

Summary

Test Plan

Check if all charts look like before
Screenshot 2022-08-11 at 18 00 46
Screenshot 2022-08-11 at 18 07 08
Screenshot 2022-08-11 at 17 56 52

@pull-request-size pull-request-size bot added the size/XXL Denotes a Pull Request that changes 1000+ lines. label Aug 11, 2022
@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 Aug 11, 2022
@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/XXL Denotes a Pull Request that changes 1000+ lines. labels Aug 11, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2022 •
edited

dp-workflow

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

Results for commit 5635e19.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2022 •
edited

nms-workflow

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

Results for commit 5635e19.

♻️ This comment has been updated with latest results.

@thmsschmitt thmsschmitt marked this pull request as ready for review August 11, 2022 16:01
@thmsschmitt thmsschmitt requested review from a team and Neudrino August 11, 2022 16:02
@Neudrino Neudrino added DONOTLAND Please don't merge this status: blocked and removed status: blocked labels Aug 12, 2022
@pull-request-size pull-request-size bot added size/XXL Denotes a Pull Request that changes 1000+ lines. and removed size/L Denotes a Pull Request that changes 100-499 lines. labels Sep 6, 2022
@magma magma deleted a comment from github-actions bot Sep 6, 2022
@Neudrino Neudrino self-assigned this Sep 6, 2022
@Neudrino Neudrino force-pushed the nms/upgrade-chart-js-2 branch 2 times, most recently from 0d2bafd to a030810 Compare September 7, 2022 13:51
@pull-request-size pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/XXL Denotes a Pull Request that changes 1000+ lines. labels Sep 7, 2022
@Neudrino Neudrino removed the DONOTLAND Please don't merge this label Sep 7, 2022
@Neudrino
Copy link
Contributor

Neudrino commented Sep 7, 2022 •
edited

@Neudrino Neudrino requested review from sebathomas and removed request for sebathomas September 7, 2022 13:53
thmsschmitt and others added 2 commits September 8, 2022 13:48
@thmsschmitt @Neudrino
chore(nms): Upgrade chart-js and react-chart-js
91470cd 
Signed-off-by: Thomas Schmitt <thomas.schmitt@tngtech.com>
@Neudrino
refactor(nms): use date-fns instead of moment for chart.js
5635e19 
Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>
@Neudrino
Copy link
Contributor

Neudrino commented Sep 8, 2022

For testing I did:

  • nms$ yarn test
  • nms$ ./e2e_test_setup.sh
  • start orc8r and nms and accessed the UI to see a diagram displayed

@sebathomas I did have some weird issue, presumably with the bundler, yesterday. However, after a clean start today, I could not systematically reproduce them. Even without code changes compared to yesterday, the tests were green for me today.
It would be good, if you could also see, whether the described tests also work fine on your machine.

@sebathomas
Copy link
Contributor

Yes, they're green for me too. Don't these tests run in the CI?

sebathomas
sebathomas approved these changes Sep 8, 2022
View reviewed changes
Copy link
Contributor

@sebathomas sebathomas left a comment

Choose a reason for hiding this comment

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

Also tried starting NMS locally and didn't notice any errors.

@Neudrino
Copy link
Contributor

Neudrino commented Sep 8, 2022

Yes, they're green for me too. Don't these tests run in the CI?

Yes they do. But as I have seen this errors yesterday, I thought one additional data point would not hurt here. Just to gain more confidence.

@Neudrino Neudrino merged commit d6cea33 into magma:master Sep 9, 2022
@Neudrino Neudrino deleted the nms/upgrade-chart-js-2 branch September 9, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebathomas sebathomas sebathomas approved these changes

@Neudrino Neudrino Awaiting requested review from Neudrino

Assignees

@Neudrino Neudrino

Labels
component: nms NMS-related issue size/L Denotes a Pull Request that changes 100-499 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade chart.js and react-chartjs-2
3 participants
@thmsschmitt @Neudrino @sebathomas