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

Bump angular dependency from 1.7.9 to 1.8.0 #74482

Merged
merged 4 commits into from
Aug 11, 2020
Merged

Bump angular dependency from 1.7.9 to 1.8.0 #74482

merged 4 commits into from
Aug 11, 2020

Conversation

watson
Copy link
Contributor

@watson watson commented Aug 6, 2020 •
edited
Loading

There's no versions in-between 1.7.9 and 1.8.0. Here's the changelog for v1.8.0: https://github.com/angular/angular.js/blob/master/CHANGELOG.md#180-nested-vaccination-2020-06-01

TL;DR: 1.8.0 only contains one change (breaking), which in short can be summarised as:

[...] no longer turns XHTML-like strings like <div /><span /> to sibling elements <div></div><span></span> when not in XHTML mode. Instead it will leave them as-is. The browser, in non-XHTML mode, will convert these to: <div><span></span></div>.

I don't believe this should be an issue for us, and the tests are also all passing, so we should be good. But I'm no angular expert, so it would be nice with a second pair of 👀

@watson watson added the release_note:skip Skip the PR/issue when compiling release notes label Aug 6, 2020
@watson watson self-assigned this Aug 6, 2020
@watson watson marked this pull request as ready for review August 6, 2020 12:33
@watson watson requested a review from a team as a code owner August 6, 2020 12:33
@watson watson requested a review from a team August 6, 2020 12:44
@legrego
Copy link
Member

legrego commented Aug 6, 2020

I recall a previous angular (or jquery?) upgrade breaking ML, so I'm going to tag them in for review here too to make sure we don't introduce a regression. It think the Kibana App team still relies on angular too, so it'd be good to have them verify manually as well.

@legrego legrego requested review from a team August 6, 2020 13:50
@jportner
Copy link
Contributor

jportner commented Aug 6, 2020 •
edited
Loading

I recall a previous angular (or jquery?) upgrade breaking ML

Yes, it was a jquery upgrade that had a breaking change. The ML plugin in 6.8 has several programmatically defined Angular templates which was the source of the problems we encountered. Since that jquery upgrade, the ML team added a few functional tests to 6.8 to hopefully catch any issues that crop up with dependency upgrades.

spalger
spalger approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM from an operations perspective, but I'm glad to hear that we're getting eyes from the ML team and Kibana APP teams

jgowdyelastic
jgowdyelastic approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML no longer contains any angular code, so this is good from our side.

@watson
Copy link
Contributor Author

watson commented Aug 6, 2020

@jportner Good to hear. I wanted to backport this to 6.8.x in the end, but wasn't sure how complicated that would me. I'll give it a shot once this is merged.

@watson watson added the v6.8.12 label Aug 6, 2020
@watson
Copy link
Contributor Author

watson commented Aug 10, 2020

@elasticmachine merge upstream

jportner
jportner approved these changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

LGTM. The breaking change described above is the same one we encountered with the jQuery upgrade described in #74482 (comment) (PR: #64884). We've already changed self-closing tags to be explicitly-closing tags where appropriate.

jportner
jportner reviewed Aug 10, 2020
View reviewed changes
package.json Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

mbondyra
mbondyra approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@watson watson merged commit ce6011e into elastic:master Aug 11, 2020
@watson watson deleted the bump-angular branch August 11, 2020 08:05
This was referenced Aug 11, 2020
Merged
Merged
watson added a commit to watson/kibana that referenced this pull request Aug 11, 2020
@watson
Bump angular dependency from 1.7.9 to 1.8.0 (elastic#74482)
9416674 
# Conflicts:
#	x-pack/package.json
@watson watson mentioned this pull request Aug 11, 2020
Merged
watson added a commit to watson/kibana that referenced this pull request Aug 11, 2020
@watson
Bump angular dependency from 1.7.9 to 1.8.0 (elastic#74482)
56b5c09 
# Conflicts:
#	packages/kbn-ui-shared-deps/package.json
#	x-pack/package.json
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 11, 2020
@gmmorris
Merge branch 'master' into event-log/use-alerts-actions-clients
bbc3999 
* master: (339 commits)
  [Ingest Node Pipelines] Sentence-case processor names (elastic#74645)
  Bump angular dependency from 1.7.9 to 1.8.0 (elastic#74482)
  [ML] Fixing schema for custom rule conditions (elastic#74676)
  [ML] Refactor in preparation for new es client (elastic#74552)
  [ML] Adding initial file analysis overrides (elastic#74376)
  Allow any hostname for chromium proxy bypass (elastic#74693)
  [ML] ML on Kibana Management: Add ability to pass a group ID filter to job management page (elastic#74533)
  [Metrics UI] Fix No Data preview pluralization (elastic#74399)
  [Bug][Security_Solution][Telemetry] Capitalize S in macOS (elastic#74688)
  Remove karma tests  from legacy maps (elastic#74668)
  [Ingest Manager] stop creating events-* index pattern and placeholder index (elastic#74683)
  [Enterprise Search] Update the browser/document title on plugin navigation (elastic#74392)
  [visualizations] Add i18n translation for 'No results found' (elastic#74619)
  [maps] convert vector style properties to TS (elastic#74553)
  bump geckodriver binary to 0.27 (elastic#74638)
  fix: update apm agents to catch abort requests (elastic#74658)
  [Security Solution] Resolver children pagination (elastic#74603)
  add memoryStatus to df analytics page and analytics table in management (elastic#74570)
  [Ingest Manager] Allow prerelease in package version (elastic#74452)
  [App Arch]: remove legacy karma tests (elastic#74599)
  ...
watson added a commit to watson/kibana that referenced this pull request Aug 11, 2020
@watson
Bump angular dependency from 1.7.9 to 1.8.0 (elastic#74482)
d22960b
@watson watson mentioned this pull request Aug 11, 2020
Closed
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ingest-pipeli…
97eea65 
…nes/processor-forms-a-d

* 'master' of github.com:elastic/kibana: (26 commits)
  [Telemetry][API Integration] size_in_bytes to be a number (elastic#74664)
  [ILM] Convert node details flyout to TS (elastic#73707)
  [Ingest Node Pipelines] Sentence-case processor names (elastic#74645)
  Bump angular dependency from 1.7.9 to 1.8.0 (elastic#74482)
  [ML] Fixing schema for custom rule conditions (elastic#74676)
  [ML] Refactor in preparation for new es client (elastic#74552)
  [ML] Adding initial file analysis overrides (elastic#74376)
  Allow any hostname for chromium proxy bypass (elastic#74693)
  [ML] ML on Kibana Management: Add ability to pass a group ID filter to job management page (elastic#74533)
  [Metrics UI] Fix No Data preview pluralization (elastic#74399)
  [Bug][Security_Solution][Telemetry] Capitalize S in macOS (elastic#74688)
  Remove karma tests  from legacy maps (elastic#74668)
  [Ingest Manager] stop creating events-* index pattern and placeholder index (elastic#74683)
  [Enterprise Search] Update the browser/document title on plugin navigation (elastic#74392)
  [visualizations] Add i18n translation for 'No results found' (elastic#74619)
  [maps] convert vector style properties to TS (elastic#74553)
  bump geckodriver binary to 0.27 (elastic#74638)
  fix: update apm agents to catch abort requests (elastic#74658)
  [Security Solution] Resolver children pagination (elastic#74603)
  add memoryStatus to df analytics page and analytics table in management (elastic#74570)
  ...
watson added a commit that referenced this pull request Aug 11, 2020
@watson
Bump angular dependency from 1.7.9 to 1.8.0 (#74482) (#74734)
45e460e
watson added a commit that referenced this pull request Aug 11, 2020
@watson
Bump angular dependency from 1.7.9 to 1.8.0 (#74482) (#74735)
2dd1b0f 
# Conflicts:
#	x-pack/package.json
watson added a commit that referenced this pull request Aug 11, 2020
@watson
Bump angular dependency from 1.7.9 to 1.8.0 (#74482) (#74737)
0398448 
# Conflicts:
#	packages/kbn-ui-shared-deps/package.json
#	x-pack/package.json
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 13, 2020
@spalger spalger mentioned this pull request Aug 13, 2020
Closed
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

3 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@watson watson removed the v6.8.12 label Aug 19, 2020
@jportner jportner added backported and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jportner jportner jportner approved these changes

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@mbondyra mbondyra mbondyra approved these changes

@spalger spalger spalger approved these changes

Assignees

@watson watson

Labels
backported release_note:skip Skip the PR/issue when compiling release notes v7.8.2 v7.9.1 v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@watson @legrego @jportner @kibanamachine @spalger @mbondyra @jgowdyelastic @elasticmachine