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

Update elastic-charts to v41.0.1 #5492

Merged
merged 10 commits into from
Dec 20, 2021
Merged

Update elastic-charts to v41.0.1 #5492

merged 10 commits into from
Dec 20, 2021

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Dec 18, 2021 •
edited
Loading

Summary

This PR upgrades @elastic/charts to v41.0.1 which included some breaking changes. Namely, the removal of config from PartitionSpec. EUI uses this config to set default properties on the shared eui charts theme, thus throws and errors with PartitionConfig being removed.

The changes move all the old config overrides into the theme.partition sections which is a 1:1 match for all defined properties.

Note: I'm not sure if y'all are able to backport or plan to use your latest version in 7.17 but this would be ideal for us as we plan to upgrade charts to the breaking version in 7.17.

@nickofthyme nickofthyme added dependencies Pull requests that update a dependency file Elastic Charts labels Dec 18, 2021
@nickofthyme nickofthyme mentioned this pull request Dec 18, 2021
Merged
1 task
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5492/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5492/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5492/

@thompsongl
Copy link
Contributor

Hmm I wonder why Renovate didn't initiate a PR for this

cchaos
cchaos approved these changes Dec 20, 2021
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🙏 Thank you @nickofthyme for handling this upgrade! I cross-checked with the current published docs and it all looks the same. @thompsongl can most likely help get a specific backport for this for 7.17. It might be good to coordinate into one PR on the Kibana side.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@thompsongl thompsongl requested review from thompsongl and removed request for chandlerprall December 20, 2021 16:09
cchaos
cchaos suggested changes Dec 20, 2021
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Erg, found one odd spot in the Copy/Paste code snippet.

src-docs/src/views/elastic_charts/pie_slices.js Outdated Show resolved Hide resolved
@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 20, 2021 •
edited
Loading

@thompsongl can most likely help get a specific backport for this for 7.17. It might be good to coordinate into one PR on the Kibana side.

That sounds good. I opened elastic/kibana#121593 to make the necessary changes in kibana but could upgrade eui with this fix in that pr.

Then I can backport the pr into 7.17 with the backported version. @thompsongl 7.17 is on 39.1.3 so not sure how you want to handle that as it is technically breaking by removing the eui partition value. I could change this to duplicate that to avoid the breaking change. LMK

Update

I added back the theme.partition value so it would be less of a breaking change here 93a4a4b. Though since the previous type PartitionConfig no longer exists in the latest charts version it would still be breaking the types. If needed I can add this as an export in charts.

@thompsongl thompsongl mentioned this pull request Dec 20, 2021
Closed
1 task
@thompsongl
Copy link
Contributor

thompsongl commented Dec 20, 2021 •
edited
Loading

I could change this to duplicate that to avoid the breaking change.

Yes, instead of removing the config value now, let's duplicate as necessary and mark it as deprecated. That will allow us to backport this upgrade to [email protected] along with any other changes we need to get into 7.17 (tracking in #5493).

We can then do a follow-up PR with the breaking change removal and get that into a normal 8.x stack release.

@nickofthyme
Copy link
Contributor Author

Ok then let me export that previous type and mark it as deprecated.

@thompsongl thompsongl added this to the Elastic Stack 7.17 milestone Dec 20, 2021
@nickofthyme nickofthyme mentioned this pull request Dec 20, 2021
Merged
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5492/

thompsongl
thompsongl suggested changes Dec 20, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
src/themes/charts/themes.ts Outdated Show resolved Hide resolved
@thompsongl thompsongl mentioned this pull request Dec 20, 2021
Open
35 tasks
@nickofthyme nickofthyme changed the title Update elastic-charts to v41.0.0 Update elastic-charts to v41.0.1 Dec 20, 2021
@nickofthyme nickofthyme enabled auto-merge (squash) December 20, 2021 17:49
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5492/

@nickofthyme nickofthyme merged commit c4bcfe2 into elastic:main Dec 20, 2021
@nickofthyme nickofthyme deleted the upgrade-charts-41-breaking branch December 20, 2021 18:52
@cee-chen cee-chen mentioned this pull request Jan 5, 2022
Merged
@cee-chen cee-chen mentioned this pull request May 4, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thompsongl thompsongl thompsongl approved these changes

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file Elastic Charts
Projects
None yet
Milestone
Elastic Stack 7.17
Development

Successfully merging this pull request may close these issues.
4 participants
@nickofthyme @kibanamachine @thompsongl @cchaos