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

Remove id attribute from canvas attributes #30736

Merged
merged 10 commits into from
Feb 14, 2019

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Feb 11, 2019

This PR removes the id attribute from canvas mappings and uses the root id attribute from saved objects (solves canvas portion of #30455). The reason ids need to be removed from attributes is to facilitate new import / export feature (development in progress) that may change the ids when conflicts exist. See #27203.

@mikecote mikecote self-assigned this Feb 11, 2019
@mikecote mikecote requested a review from a team as a code owner February 11, 2019 21:51
@mikecote mikecote changed the title [wip] Remove id attribute from canvas attributes Remove id attribute from canvas attributes Feb 11, 2019
@mikecote
Copy link
Contributor Author

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mikecote
Copy link
Contributor Author

Jenkins, test this

@mikecote mikecote added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas and removed WIP Work in progress labels Feb 12, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

Pulled this down and everything still works the same. I also confirmed that the mapping is updated and that the id property is removed. All of the existing workpad documents also no longer have the id property.

LGTM!

@w33ble
Copy link
Contributor

w33ble commented Feb 13, 2019

@mikecote actually, one small thing for you to add to this PR. All the sample workpads still have an id property on the workpad, and those need to be removed as well.

See the JSON files in x-pack/plugins/canvas/server/sample_data

Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

Please update the sample data JSON

@mikecote mikecote requested a review from w33ble February 13, 2019 22:59
@@ -4,10 +4,11 @@
"type": "canvas-workpad",
"updated_at": "2018-10-22T15:19:02.081Z",
"version": 1,
"migrationVersion": {},
"migrationVersion": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since I don't know what this is used for, I might as well ask... what is this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to indicate to the migrator what version the data is migrated up to. Having {} indicates that the system needs to run all the canvas-workpad migrations when creating the record. Having 7.0.0 value here indicates to the migrator that the record is migrated up to version 7.0.0.

In this scenario the migrator won't need to run any migrations since the id field is already removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, but it allows us to have, for example, an 8.0.0 migration in the future, and it would only run that since it knows it's already on 7.0.0. That's neat, thanks for the explanation!

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Feb 14, 2019

Sorry, you'll need to merge master to get rid of those OOM errors

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mikecote mikecote merged commit d4022d8 into elastic:master Feb 14, 2019
mikecote added a commit to mikecote/kibana that referenced this pull request Feb 14, 2019
* Remove id attribute from canvas attributes

* Fix create and update API

* Add migration tests

* Use constants

* Add API tests

* Cleanup tests

* Apply PR feedback
mikecote added a commit to mikecote/kibana that referenced this pull request Feb 14, 2019
* Remove id attribute from canvas attributes

* Fix create and update API

* Add migration tests

* Use constants

* Add API tests

* Cleanup tests

* Apply PR feedback
mikecote added a commit that referenced this pull request Feb 14, 2019
* Remove id attribute from canvas attributes

* Fix create and update API

* Add migration tests

* Use constants

* Add API tests

* Cleanup tests

* Apply PR feedback
mikecote added a commit that referenced this pull request Feb 14, 2019
* Remove id attribute from canvas attributes

* Fix create and update API

* Add migration tests

* Use constants

* Add API tests

* Cleanup tests

* Apply PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants