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

Convert default_watch.json to a JS object in order to avoid TS complaints #89488

Merged
merged 2 commits into from
Jan 28, 2021
Merged

Convert default_watch.json to a JS object in order to avoid TS complaints #89488

merged 2 commits into from
Jan 28, 2021

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jan 27, 2021 •
edited
Loading

Resolves #89321 (comment), which is blocking TS project migration of the watcher plugin.

To test, create an Advanced Watch in Watcher and verify that it is prepopulated with watch JSON.

@cjcenizal cjcenizal added chore Feature:Watcher v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 27, 2021
@cjcenizal cjcenizal requested a review from a team as a code owner January 27, 2021 21:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

cjcenizal
cjcenizal commented Jan 27, 2021
View reviewed changes
x-pack/plugins/watcher/public/application/models/watch/json_watch.js Outdated
@@ -113,7 +114,7 @@ export class JsonWatch extends BaseWatch {
return new JsonWatch(upstreamWatch);
}

static defaultWatchJson = defaultWatchJson;
static defaultWatch = defaultWatch;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewers, I need to do a quick grep and see if this renaming will cause any problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this isn't referenced anywhere in the codebase, so I removed it.

alisonelizabeth
alisonelizabeth approved these changes Jan 27, 2021
View reviewed changes
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.

Code LGTM. Verified locally. Thanks for addressing this!

TinaHeiligers
TinaHeiligers reviewed Jan 27, 2021
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'll pull this into the branch for migrating watcher to a TS project and give feedback thereafter.

LGTM from a code POV. I haven't run the code.

Not directly related to this work but still needed for migrating the plugin: could we type WATCH explicitly please? ATM, the TS project build throws an error for that.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
watcher 887.6KB 887.4KB -188.0B

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

TinaHeiligers
TinaHeiligers approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I pulled the code into the branch for converting watcher to a TS project and it solves the issue of the .json file. The only other issue is:

Exported variable 'WATCH' has or is using name 'Watch' from external module "/Users/christianeheiligers/Projects/kibana/x-pack/plugins/watcher/__fixtures__/watch" but cannot be named.

11 export const WATCH = { watch: getWatch({ id: WATCH_ID }) };

but I could get around that with:
export const WATCH: any = { watch: getWatch({ id: WATCH_ID }) };
LGTM!

@cjcenizal cjcenizal merged commit fd2e9d0 into elastic:master Jan 28, 2021
@cjcenizal cjcenizal deleted the chore/watcher-default-watch-json branch January 28, 2021 00:21
@cjcenizal
Copy link
Contributor Author

Thanks @TinaHeiligers I'll let you make that change in your PR so that the related changes are made in a single commit.

@cjcenizal cjcenizal mentioned this pull request Jan 28, 2021
Merged
cjcenizal added a commit that referenced this pull request Jan 28, 2021
@cjcenizal
Convert default_watch.json to a JS object in order to avoid TS compla…
78d7dca 
…ints (#89488) (#89515)

* Remove unused defaultWatchJson static member from public JsonWatch model.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/basic-a11…
e814056 
…y-tests

* 'master' of github.com:elastic/kibana: (31 commits)
  [Discover] Add grid flyout jest test (elastic#89088)
  [Search Sessions] Improve session restoration back button (elastic#87635)
  [TSVB] Remove vis_type_timeseries_enhanced plugin (elastic#89274)
  [Security Solution] Init Osquery plugin (elastic#87109)
  [Fleet] Do not defined aliases inside datastream template (elastic#89512)
  skip flaky suite (elastic#86950)
  chore(NA): bazel machinery installation on kbn bootstrap (elastic#89469)
  [build/docker] Add support for centos ARM builds (elastic#84831)
  Convert default_watch.json to a JS object in order to avoid TS complaints (elastic#89488)
  [CI] Decrease number of Jest workers (elastic#89504)
  [Maps] remove maps_oss TS project (elastic#89502)
  Adds migration settings to Docker (elastic#89501)
  [Lens] Fix crash in transition from unique count to last value (elastic#88916)
  [kbn-es] Always use bundled JDK when starting Elasticsearch (elastic#89437)
  unskip getting_started/shakespeare test elasticsearch 64016 (elastic#89346)
  [Maps] migrate maps, maps_file_upload, and maps_legacy_licensing to TS projects (elastic#89439)
  skip flaky suite (elastic#89478)
  skip flaky suite (elastic#89476)
  skip flaky suite (elastic#89477)
  skip flaky suite (elastic#89475)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/rollover-…
494a041 
…updates-and-timeline-cleanup

* 'master' of github.com:elastic/kibana: (44 commits)
  [Discover] Add grid flyout jest test (elastic#89088)
  [Search Sessions] Improve session restoration back button (elastic#87635)
  [TSVB] Remove vis_type_timeseries_enhanced plugin (elastic#89274)
  [Security Solution] Init Osquery plugin (elastic#87109)
  [Fleet] Do not defined aliases inside datastream template (elastic#89512)
  skip flaky suite (elastic#86950)
  chore(NA): bazel machinery installation on kbn bootstrap (elastic#89469)
  [build/docker] Add support for centos ARM builds (elastic#84831)
  Convert default_watch.json to a JS object in order to avoid TS complaints (elastic#89488)
  [CI] Decrease number of Jest workers (elastic#89504)
  [Maps] remove maps_oss TS project (elastic#89502)
  Adds migration settings to Docker (elastic#89501)
  [Lens] Fix crash in transition from unique count to last value (elastic#88916)
  [kbn-es] Always use bundled JDK when starting Elasticsearch (elastic#89437)
  unskip getting_started/shakespeare test elasticsearch 64016 (elastic#89346)
  [Maps] migrate maps, maps_file_upload, and maps_legacy_licensing to TS projects (elastic#89439)
  skip flaky suite (elastic#89478)
  skip flaky suite (elastic#89476)
  skip flaky suite (elastic#89477)
  skip flaky suite (elastic#89475)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/lib/absolute_timing_to_relative_timing.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
chore Feature:Watcher release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Meta] Migrate plugins to TS references for es_ui CODEOWNERS plugins
5 participants
@cjcenizal @elasticmachine @kibanamachine @alisonelizabeth @TinaHeiligers