[Management] Enable index pattern version conflicts #18937
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
jen-huang
requested changes
May 11, 2018
There was a problem hiding this comment.
I can't get formatting to be cleared using two tabs like before, so that issue is taken care of! There are still two issues that I see:
Refresh requires two clicks
- Open two tabs to index pattern fields list.
- In tab 1, change a field's format to anything other than
default, save. - In tab 2, click refresh fields - list is not updated with new format. Click refresh again - list is updated.
Default format can be overridden with old version
- Open two tabs to index pattern fields list - with at least one field that has a format other than default (will use
Numberfor example) - In tab 1, change that field's
Numberformat todefault, save. - In tab 2, click refresh fields.
- In tab 1, reload page - that field's format is back to
Number.
💔 Build Failed |
|
@jen-huang Great finds! After investigation, I opened #19037 to describe some of the issues I saw related to this. 94c71bb should address your issues as well as the earlier mentioned issue! |
💔 Build Failed |
|
jenkins, test this |
💔 Build Failed |
|
jenkins, test this |
💔 Build Failed |
|
Tests pass locally. This is a bummer jenkins, test this |
💔 Build Failed |
…y and resolve if we can
…nd force a fields refresh. This ensures we pick up on field format (and other) changes
chrisronline
force-pushed
the
index_pattern_version_conflicts
branch
from
94c71bb
to
af6fdda
Compare
💚 Build Succeeded |
jen-huang
approved these changes
May 17, 2018
There was a problem hiding this comment.
@chrisronline Those changes fixed the two issues I was seeing! And I'm not seeing the issues described in #19037. Resetting to default works fine for me.
However, I am seeing an issue where versioning isn't kicking in once a field has been changed. Steps:
- Open two tabs to the same index pattern
- In tab 1, click Edit on a string field, change its format to String and save
- In tab 2, click refresh fields (the new String format for the edited field shows correctly)
- Stay in tab 2, click Edit in the same updated field, change its format to Truncated String and save
- In tab 1, DO NOT REFRESH FIELDS, click edit in the same field, change its format from String (outdated since we didn't refresh) to Url
- Observe that the save was successful. I would expect the toast about having to refresh fields to pop up at this point.
However, this might be an edge case. I think we've resolved the biggest issue of refreshing fields overwriting changes already, so PR LGTM!
|
@jen-huang That's a legit bug! Great find! |
💚 Build Succeeded |
|
✅ Confirmed changes fixed the bug! |
chrisronline
added a commit
to chrisronline/kibana
that referenced
this pull request
May 22, 2018
* Pass the version along so we can get version conflict errors, then try and resolve if we can * Update the version post save * Refactor slightly to match setId pattern * Tests and updates to ensure the actual changes aren't clobbered * Ensure we return the id * Avoid infinite recursion, welcome suggestions on how to unit test this * Change logic for refresh fields UI button. Now it will re-call init and force a fields refresh. This ensures we pick up on field format (and other) changes * Fix a couple issues with saving field formats, elastic#19037 * Use the right key for version
chrisronline
added a commit
that referenced
this pull request
May 22, 2018
* Pass the version along so we can get version conflict errors, then try and resolve if we can * Update the version post save * Refactor slightly to match setId pattern * Tests and updates to ensure the actual changes aren't clobbered * Ensure we return the id * Avoid infinite recursion, welcome suggestions on how to unit test this * Change logic for refresh fields UI button. Now it will re-call init and force a fields refresh. This ensures we pick up on field format (and other) changes * Fix a couple issues with saving field formats, #19037 * Use the right key for version
|
Backport: 6.x: 55b8130 |
This was referenced Jun 19, 2018
This was referenced Sep 11, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #18584, #15738, #19037
Summary
This PR adds support for index patterns to leverage optimistic concurrency which is already supported through the saved objects api.
The basic flow is:
Testing
There are three main test cases I've been using to test this:
Changing a field format
Not changing a field format in the second tab
Using discover
AddorRemoveon any of the fields on the left (ensure it's the same index pattern). It should successfully go through and the field formatting changes made in step 3 should be preserved (verify by refreshing the page on tab 1 and clicking edit again)Questions/Areas of Note
The code around trying to resolve the conflicts manually feels a bit trappy but it felt like a good use case to support to avoid unnecessary page refreshes (honestly, maybe needing to refresh the page is up for debate too?). All reviewers: please spend extra time reviewing this. I added a unit test but would love some more validation here.
cc @tylersmalley