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

SerialConsole: Use xterm v4 #3030

Merged
merged 2 commits into from Oct 8, 2019
Merged

Conversation

@marusak
Copy link
Contributor

marusak commented Sep 27, 2019

What: Use xterm v4 in SerialConsole. Fixes #2912

Fixes #2912
@marusak marusak mentioned this pull request Sep 27, 2019
1 of 1 task complete
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 27, 2019

PatternFly-React preview: https://patternfly-react-pr-3030.surge.sh

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #3030 into master will decrease coverage by 0.03%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3030      +/-   ##
==========================================
- Coverage   68.98%   68.94%   -0.04%     
==========================================
  Files         857      852       -5     
  Lines       23421    23284     -137     
  Branches     1819     1790      -29     
==========================================
- Hits        16156    16053     -103     
+ Misses       6365     6343      -22     
+ Partials      900      888      -12
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.17% <50%> (ø) ⬆️
#patternfly4 67.99% <ø> (-0.1%) ⬇️
Impacted Files Coverage Δ
...ternfly-3/react-console/src/SerialConsole/XTerm.js 27.86% <50%> (+2.44%) ⬆️
...ct-core/src/components/Dropdown/DropdownToggle.tsx 84.84% <0%> (-3.04%) ⬇️
...eact-core/src/components/Dropdown/DropdownItem.tsx 70.58% <0%> (-1.64%) ⬇️
...rnfly-3/patternfly-react/src/components/Nav/Nav.js 43.24% <0%> (-0.35%) ⬇️
...eact-core/src/components/Pagination/Navigation.tsx 87.95% <0%> (-0.15%) ⬇️
...nfly-react/src/components/Pagination/Pagination.js 27.83% <0%> (ø) ⬆️
...rc/components/Pagination/PaginationOptionsMenu.tsx 93.54% <0%> (ø) ⬆️
...nfly-4/react-core/src/components/Dropdown/index.ts 100% <0%> (ø) ⬆️
...eact-core/src/components/Pagination/Pagination.tsx 82.53% <0%> (ø) ⬆️
...rnfly-4/react-table/src/components/Table/Table.tsx 93.24% <0%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1bd56f...3d1b2c7. Read the comment docs.

@marusak

This comment has been minimized.

Copy link
Contributor Author

marusak commented Oct 2, 2019

Can I get a review on this? (not really sure who to ping, maybe @mareklibra as he has some commits in serial console)

@tlabaj tlabaj requested review from redallen and jeff-phillips-18 Oct 2, 2019
@jeff-phillips-18 jeff-phillips-18 requested a review from mareklibra Oct 2, 2019
@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Oct 2, 2019

code changes look ok to me but I don't really know this package all that well. If @mareklibra is OK with it then I am.

@tlabaj tlabaj self-assigned this Oct 3, 2019
Copy link
Member

dlabrecq left a comment

Is this a breaking change? That is, considering users must now move to xterm-4.0.0, change their imports, etc.?

@marusak

This comment has been minimized.

Copy link
Contributor Author

marusak commented Oct 4, 2019

yes, users needs to move xterm to 4.0.0. But otherwise there should be no other visible change nor they do not need to adjust their code in any way.

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Oct 4, 2019

As long as it's not considered a breaking change, I believe it would be ok.

@martinpitt

This comment has been minimized.

Copy link

martinpitt commented Oct 8, 2019

Sorry for nagging, but is there anything else to be done on our (@marusak 's) side? This seems approved and all tests passed. Thanks!

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Oct 8, 2019

Should this be a breaking change? Regardless, the commit message needs to be updated for semantic release. If breaking, then please use major otherwise feat would do.

@marusak

This comment has been minimized.

Copy link
Contributor Author

marusak commented Oct 8, 2019

Should this be a breaking change?

I answered this in a previous comment

Regardless, the commit message needs to be updated for semantic release. If breaking, then please use major otherwise feat would do.

Can this be done on squashing? If it can, then I don't need to push force and then tests don't need to re-run. Otherwise I can edit the commit message (feat(SerialConsole): Use xterm v4 would work, I guess?)

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Oct 8, 2019

Sorry, I guess my question is, if adopters are forced to move to xterm 4.0.0 then wouldn't this be a breaking change?

@marusak

This comment has been minimized.

Copy link
Contributor Author

marusak commented Oct 8, 2019

Sorry, I guess my question is, if adopters are forced to move to xterm 4.0.0 then wouldn't this be a breaking change?

I don't know what is defined as breaking change. It requires users to update xterm to 4.0.0, but otherwise they should not notice any difference and they do not need to update their code (unless they use xterm on other places as well).

@jeff-phillips-18

This comment has been minimized.

Copy link
Member

jeff-phillips-18 commented Oct 8, 2019

I'm going to flag this for a major release as it would break consumers if they pull this change and do nothing else.

BREAKING CHANGE: bump xterm from ^3.3.0 to ^4.0.0
Copy link
Contributor

redallen left a comment

I added an empty commit with a proper message for a major release for @patternfly/react-console.

@jeff-phillips-18 jeff-phillips-18 merged commit 383ec51 into patternfly:master Oct 8, 2019
7 of 8 checks passed
7 of 8 checks passed
ci/circleci: lint Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 8, 2019

Your changes have been released in:

  • patternfly-react-extensions@3.0.0
  • @patternfly/react-console@2.0.0
  • @patternfly/react-core@3.113.2
  • @patternfly/react-docs@4.14.13
  • @patternfly/react-inline-edit-extension@2.11.81
  • demo-app-ts@3.6.20
  • @patternfly/react-table@2.22.30
  • @patternfly/react-topology@2.8.75
  • @patternfly/react-virtualized-extension@1.2.65

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.