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

chore(NA): assures a single version for the same dependency across the entire project #78825

Merged
merged 22 commits into from
Oct 1, 2020

Conversation

mistic
Copy link
Member

@mistic mistic commented Sep 29, 2020

That PR completes the work initiated on #78327
It clears the list of remaining dependencies with different versions across the repository.
Additionally it also introduces a script to make sure we will keep that way until we completely land our work to migrate into a single package json #76412

@mistic mistic added chore Team:Operations Team label for Operations Team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 29, 2020
@mistic mistic requested a review from tylersmalley September 29, 2020 17:20
@tylersmalley
Copy link
Contributor

Will still need to hook it up to CI. You can actually remove the Grunt config and use checks-reporter-with-killswitch in a script inside test/scripts/ to be called by Jenkins.

@mistic
Copy link
Member Author

mistic commented Sep 29, 2020

@tylersmalley thanks for spotting it. I've changed it

@mistic mistic marked this pull request as ready for review September 29, 2020 21:18
@mistic mistic requested review from a team as code owners September 29, 2020 21:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

APM changes look good

@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Sep 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@spalger
Copy link
Contributor

spalger commented Sep 30, 2020

Why not integrate this into bootstrap? All the data necessary to do this check is available in https://github.com/elastic/kibana/blob/master/packages/kbn-pm/src/utils/validate_yarn_lock.ts and if it was integrated into bootstrap you and others wouldn't need to rely on CI to validate this, it'd be validated on every install very quickly.

@mistic mistic force-pushed the single-version-deps branch from 6ff1cda to 0a869dc Compare September 30, 2020 03:09
@mistic mistic force-pushed the single-version-deps branch from 3454043 to eb919d0 Compare September 30, 2020 17:03
@spalger
Copy link
Contributor

spalger commented Sep 30, 2020

it would be better to just not add extra time to the bootstrap in such a temporary thing.

I think that if we expect this check to catch actually cases of people accidentally adding duplicate dependencies we should try to give the feedback as soon as possible, and all the tools are available there to check the package.json files for all packages without any perceptible time cost. It doesn't need to look at transitive dependencies, we have all the package.json files already parsed and in memory, this would simply be a little loop we add to the validation logic.

With the logic here: https://gist.github.com/spalger/dbaaef5bc1499fa27a92316b4ba758f1

We would get the following any time we bootstrap, alerting people to the problem as quickly as possible. There doesn't seem to be any good reason not to do this.

yarn run v1.22.5
$ node scripts/kbn bootstrap
 info [kibana] running yarn

$ node ./preinstall_check
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@^3.0.1"
warning Resolution field "schema-utils@1.0.0" is incompatible with requested version "schema-utils@^0.3.0"
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@~3.7.2"
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@^3.3.3333"
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@^3.2.2"
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@^3.0.3"
warning Resolution field "typescript@4.0.2" is incompatible with requested version "typescript@^3.4.5"
success Already up-to-date.

ERROR [single_version_dependencies] Multiple version ranges for the same dependency
      were found declared across different package.json files. Please consolidate
      those to match across all package.json files. Different versions for the
      same dependency is not supported.

      If you have questions about this please reach out to the operations team.

      The conflicting dependencies are:

        @types/strip-ansi
          ^3.0.0 => kibana, @kbn/pm
          ^5.2.1 => @kbn/test
        @types/webpack
          ^4.41.21 => kibana
          ^4.41.3 => @kbn/optimizer
          ^4.41.5 => @kbn/storybook
        react-redux
          ^7.2.0 => kibana, x-pack
          ^5.1.2 => @kbn/ui-framework
        redux
          ^4.0.5 => kibana, x-pack
          3.7.2 => @kbn/ui-framework
        strip-ansi
          ^3.0.1 => kibana
          ^6.0.0 => @kbn/dev-utils
          ^4.0.0 => @kbn/pm
          ^5.2.0 => @kbn/test
        tape
          ^4.13.0 => kibana
          ^5.0.1 => @elastic/safer-lodash-set
        @elastic/elasticsearch
          7.9.1 => kibana
          7.9.0-rc.1 => @kbn/es
        chalk
          ^2.4.2 => kibana
          ^4.1.0 => @kbn/dev-utils, @kbn/es, @kbn/pm, @kbn/test, @kbn/ui-framework, x-pack
        react-router
          ^5.2.0 => kibana, @kbn/ui-shared-deps, x-pack
          ^3.2.0 => @kbn/ui-framework
        rison-node
          1.0.2 => kibana
          0.3.1 => x-pack
        tsd
          ^0.13.1 => @elastic/safer-lodash-set, @kbn/config-schema, @kbn/config, @kbn/std, @kbn/utility-types
          ^0.7.4 => @kbn/apm-config-loader
        @types/loader-utils
          ^1.1.3 => @kbn/optimizer
          ^2.0.1 => @kbn/storybook
        inquirer
          ^7.3.3 => @kbn/plugin-generator
          ^1.2.2 => @kbn/plugin-helpers
        extract-zip
          ^2.0.1 => @kbn/plugin-helpers
          ^1.7.0 => x-pack
        graphql
          ^14.0.0 => @kbn/release-notes
          ^0.13.2 => x-pack
        graphql-tag
          ^2.10.3 => @kbn/release-notes
          ^2.9.2 => x-pack
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@mistic mistic force-pushed the single-version-deps branch from 4290370 to aca6579 Compare September 30, 2020 17:43
@mistic
Copy link
Member Author

mistic commented Sep 30, 2020

Alright @spalger made the need changes to get this into the bootstrap a merge button away 😄 what do you think about that proposal @tylersmalley https://github.com/mistic/kibana/pull/13/files ?

@tylersmalley
Copy link
Contributor

tylersmalley commented Sep 30, 2020

@mistic, I am fine with @spalger's proposal. This check should be short-lived as it's only to span the time until we get to a single package.json so I would rather not spend more time than is needed discussing it.

In the long term, we should be very careful with where we place these checks. There is a tradeoff for having these checks in the bootstrap, which happens numerous times a day for each developer. I understand there is a point to faster feedback, but it shouldn't come at a high cost.

@mistic
Copy link
Member Author

mistic commented Sep 30, 2020

@tylersmalley @spalger let's move on with the @spalger proposal here. I will add a note to make sure we will remove that check once we move into a single package.json

Co-authored-by: spalger <spalger@users.noreply.github.com>
@spalger
Copy link
Contributor

spalger commented Sep 30, 2020

There is a tradeoff for having these checks in the bootstrap

I don't think that's the case for any validation of static properties of the yarn.lock or package.json files personally. We would have to do something very wrong in order to increase the time bootstrap takes by even a second for these types of checks.

@mistic I think we should also remove the verifyDependencyVersions grunt task in this PR

@mistic
Copy link
Member Author

mistic commented Sep 30, 2020

@mistic I think we should also remove the verifyDependencyVersions grunt task in this PR

good call @spalger. I will take care of it

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

@mistic
Copy link
Member Author

mistic commented Sep 30, 2020

@elasticmachine merge upstream

@mistic
Copy link
Member Author

mistic commented Oct 1, 2020

@elasticmachine merge upstream

Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

SIEM/Endopoint changes LGTM!

@mistic
Copy link
Member Author

mistic commented Oct 1, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
ml 1217 1216 -1

async chunks size

id before after diff
apm 4.1MB 4.1MB -11.0B
canvas 1.5MB 1.5MB -11.0B
enterpriseSearch 430.1KB 430.1KB -2.0B
graph 1.3MB 1.3MB -11.0B
ml 10.6MB 10.6MB -2.5KB
securitySolution 10.3MB 10.3MB -11.0B
transform 1.2MB 1.2MB -11.0B
uptime 1.5MB 1.5MB -11.0B
total -2.6KB

distributable file count

id before after diff
default 45823 45812 -11
oss 26534 26519 -15

page load bundle size

id before after diff
infra 179.0KB 178.9KB -11.0B
maps 165.0KB 165.0KB -11.0B
reporting 164.5KB 164.5KB -11.0B
uiActionsEnhanced 321.0KB 321.0KB -11.0B
upgradeAssistant 64.8KB 64.8KB -2.0B
total -46.0B

History

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

@mistic mistic merged commit e5d8d49 into elastic:master Oct 1, 2020
@mistic
Copy link
Member Author

mistic commented Oct 1, 2020

7.x: 86f628d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Operations Team label for Operations Team v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants