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

declare kbn/monaco dependency on kbn/i18n explicitly #84660

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Dec 1, 2020

Summary

after #80577 yarn kbn bootstrap fails locally with

ERROR [bootstrap] failed:
ERROR Error: Command failed with exit code 1: /usr/local/Cellar/yarn/1.21.1/libexec/bin/yarn.js run kbn:bootstrap
      error Command failed with exit code 1.
      error Command failed with exit code 1.
      $ yarn build --dev
      $ node ./scripts/build.js --dev
       info Deleting old output
       info [worker] > webpack --config /Users/mikhailshustov/work/kibana/packages/kbn-monaco/webpack.config.js --env.dev
       info [worker] exited with 0 after a few seconds
       info [tsc   ] > tsc
       proc [tsc   ] src/painless/worker/lib/autocomplete.ts(20,22): error TS7016: Could not find a declaration file for module '@kbn/i18n'. '/Users/mikhailshustov/work/kibana/packages/kbn-i18n/target/node/index.js' implicitly has an 'any' type.
       proc [tsc   ]   Try `npm i --save-dev @types/kbn__i18n` if it exists or add a new declaration (.d.ts) file containing `declare module '@kbn/i18n';`
      ERROR UNHANDLED ERROR
      ERROR Error: [tsc   ] exited with code 2

because @kbn/monaco imports @kbn/i18n but doesn't list it in dependencies section

@mshustov mshustov requested a review from a team as a code owner December 1, 2020 15:27
@mshustov mshustov added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 chore labels Dec 1, 2020
@mshustov
Copy link
Contributor Author

mshustov commented Dec 1, 2020

@elastic/kibana-operations is it possible to detect such cases on CI?

@spalger
Copy link
Contributor

spalger commented Dec 1, 2020

We definitely run yarn kbn bootstrap in CI, do you have any idea why this isn't failing when CI bootstraps?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

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.

Looks like the issue is a race condition between the bootstrap step for @kbn/i18n and @kbn/monaco. Because they have similar dependencies the bootstrap scripts start at similar times and on CI it seems that @kbn/i18n finished first causing the @kbn/monaco bootstrap task to complete successfully. The only way to test and prevent this would be to use something like eslint and validate that all dependencies are declared, which we can consider, but it's not going to be an easy thing to turn on and will likely require some code to get working.

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this!

@spalger spalger merged commit 68decb8 into elastic:master Dec 1, 2020
spalger pushed a commit to spalger/kibana that referenced this pull request Dec 1, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 1, 2020
* master: (63 commits)
  Revert the Revert of "[Alerting] renames Resolved action group to Recovered (elastic#84123)"  (elastic#84662)
  declare kbn/monaco dependency on kbn/i18n explicitly (elastic#84660)
  Remove unscripted fields from sample data index-pattern saved objects (elastic#84659)
  [ML] Fix unnecessary trigger of wildcard field type search for ML plugin routes. (elastic#84605)
  Update create.asciidoc (elastic#84046)
  [Security Solution][Detections] Fix labels and issue with mandatory fields (elastic#84525)
  Fix flaky test suite (elastic#84602)
  [Security Solution] [Detections] Create a 'partial failure' status for rules (elastic#84293)
  Revert "[Alerting] renames Resolved action group to Recovered (elastic#84123)"
  Update code-comments describing babel plugins (elastic#84622)
  [Security Solution] [Cases] Cypress for case connector selector options (elastic#80745)
  [Discover] Unskip doc table tests (elastic#84564)
  [Lens] (Accessibility) Improve landmarks in Lens (elastic#84511)
  [Lens] (Accessibility) Focus mistakenly stops on righthand form (elastic#84519)
  Return early when parallel install process detected (elastic#84190)
  [Security Solution][Detections] Support arrays in event fields for Severity/Risk overrides (elastic#83723)
  [Security Solution][Detections] Fix grammatical error in validation message for threshold field in "Create new rule" -> "Define rule" (elastic#84490)
  [Fleet] Update agent details page  (elastic#84434)
  adding documentation of use of NODE_EXTRA_CA_CERTS env var (elastic#84578)
  [Search] Integrate "Send to background" UI with session service (elastic#83073)
  ...
spalger pushed a commit that referenced this pull request Dec 1, 2020
…84683)

Co-authored-by: Mikhail Shustov <restrry@gmail.com>
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 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants