Skip to content

doing config migration during config get if necessary#23377

Merged
bmcconaghy merged 2 commits intoelastic:masterfrom
bmcconaghy:fix_issue_with_config_upgrage
Oct 16, 2018
Merged

doing config migration during config get if necessary#23377
bmcconaghy merged 2 commits intoelastic:masterfrom
bmcconaghy:fix_issue_with_config_upgrage

Conversation

@bmcconaghy
Copy link
Copy Markdown
Contributor

Closes #22541

Fixing issue with migration and config not getting migrated to new version. This code makes it so if a config is missing, it gets migrated during _read.

@bmcconaghy bmcconaghy requested a review from spalger September 20, 2018 19:09
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@bmcconaghy
Copy link
Copy Markdown
Contributor Author

retest

1 similar comment
@bmcconaghy
Copy link
Copy Markdown
Contributor Author

retest

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@njd5475
Copy link
Copy Markdown
Contributor

njd5475 commented Sep 28, 2018

I've been looking into a specific edge case with this fix. Where by a read-only user is the first user to try to use the system and as such the code using the savedObjectClient would be unable to make changes to the kibana index to do the migration of the config.

@epixa And I are thinking about alternatives and options for getting this code to trigger the migration or handling the migration elsewhere or even options to removing the necessity of migrating configs between versions.

I guess the question is is the current code at least useful for some users as is and if so when should we merge it.

@bmcconaghy
Copy link
Copy Markdown
Contributor Author

I think the most common scenario is that the admin of the ES stack will be the one to do the upgrade and would likely be the first to log in to Kibana and thus this code would do its work and all is good. So I think this code can be merged as is, and the better fix could follow in a subsequent PR.

Copy link
Copy Markdown
Contributor

@njd5475 njd5475 left a comment

Choose a reason for hiding this comment

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

This will help catch one of the migration edge cases. And should enhance the user experience.

@bmcconaghy bmcconaghy merged commit 0b4f82d into elastic:master Oct 16, 2018
bmcconaghy added a commit to bmcconaghy/kibana that referenced this pull request Oct 16, 2018
* doing config migration during config get if necessary

* fixing issue with writing new config when user does not have write privilege
bmcconaghy added a commit to bmcconaghy/kibana that referenced this pull request Oct 16, 2018
* doing config migration during config get if necessary

* fixing issue with writing new config when user does not have write privilege
buildNum: this._buildNum,
log: this._log,
});
} catch (e) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should really be checking this error to make sure it's what we expect. Otherwise we could be swallowing errors that indicate unexpected issues down the road.

spalger pushed a commit that referenced this pull request Oct 16, 2018
spalger pushed a commit that referenced this pull request Oct 16, 2018
* Revert "Proofreading edits, UI text consistency (#24016)"

This reverts commit f59f1cc.

* Revert "Skip these tests (#24085)"

This reverts commit e79533d.

* Revert "Point at kibana directory for plugin loading (#24049)"

This reverts commit 3a7f5f5.

* Revert "Translate pie and vaslib_basic_options (#23761)"

This reverts commit 1f73ea1.

* Revert "doing config migration during config get if necessary (#23377)"

This reverts commit 0b4f82d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants