Skip to content

Conversation

@alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Jul 16, 2021

This PR creates a new status API that cloud can use to fetch the upgrade status for Kibana and ES.

Fixes #101282

Example responses:

GET /api/upgrade_assistant/status

{
  "readyForUpgrade": false,
  "details": "You have 3 Elasticsearch deprecation issues and 2 Kibana deprecation issues that must be resolved before upgrading."
}

{
  "readyForUpgrade": true,
   "details": "All deprecation issues have been resolved."
}

Note: GET /api/upgrade_assistant/status previously returned the list of ES deprecations, this route has been renamed to GET /api/upgrade_assistant/es_deprecations.

@alisonelizabeth alisonelizabeth added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Upgrade Assistant v7.15.0 labels Jul 16, 2021
@alisonelizabeth alisonelizabeth marked this pull request as ready for review July 27, 2021 18:05
@alisonelizabeth alisonelizabeth requested review from a team as code owners July 27, 2021 18:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Thanks for working this out @alisonelizabeth! Stack management changes lgtm. Tested locally and code looks good. Left a non-blocking suggestion of adding a few more tests for the getKibanaUpgradeStatus function.

import { DeprecationsServiceStart, IScopedClusterClient } from 'kibana/server';
import { DomainDeprecationDetails, SavedObjectsClientContract } from 'src/core/server/types';

export const getKibanaUpgradeStatus = async ({
Copy link
Member

Choose a reason for hiding this comment

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

Should we add some tests for this the same way we have for es_deprecation_status?

Comment on lines 155 to 157
getAllDeprecations: (dependencies: GetDeprecationsContext) =>
this.deprecationsFactory.getAllDeprecations(dependencies),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of requiring the calling plugin to provide these dependencies, I think we should add this to our RequestHandlerContext and automatically wire up the ES client and SO client for the current request. This would be more in line with how we expose other services.

The type for RequestHandlerContext is defined in src/core/server/index.ts and implementation for our context provider is available in src/core/server/core_route_handler_context.ts. I'd expect that we add a new deprecations key under RequestHandlerContext['core'] and use the lazy-instantiation approach we're using for the other clients in the implementation.

If we go with this approach, I don't believe we'll need to expose this 'raw' API to plugins and can keep this API internal (don't include it in plugin_context.ts).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Thanks @joshdover for the feedback! Can you let me know if this is what you had in mind: 993773e? I will follow up with updating tests and docs in a separate commit if you're happy with the approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, this looks perfect! Feel free to ping me again when the tests are in place. Thanks for this!

// We're not able to easily test different upgrade status scenarios (there are tests with mocked data to handle this)
// so, for now, we simply verify the response returns the expected format
expectedResponseKeys.forEach((key) => {
expect(body[key]).to.not.equal(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Nit; maybe we can test the kibana status at least? we have a core_deprecations functional plugin in test/plugin_functional/plugins/core_plugin_deprecations which we can use to test this endpoint in test/plugin_functional/test_suites/upgrade_assistant

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM with 1 nit about testing the endpoint

@alisonelizabeth
Copy link
Contributor Author

Hi @joshdover! Would you mind taking another look at this PR when you get a chance? Cleaned up a few things and updated the tests and docs.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Needs a couple more tests and we should be good!

}
}

class CoreDeprecationsRouteHandlerContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we update src/core/server/core_route_handler_context.test.ts to include tests for this?

public start() {}
public start(): InternalDeprecationsServiceStart {
return {
asScopedToClient: this.createScopedDeprecations(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test via da04f4b. Let me know if you think there should be anything more tested here.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@alisonelizabeth
Copy link
Contributor Author

@joshdover Added some additional tests via da04f4b. Let me know what you think. Thanks!

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@Bamieh
Copy link
Member

Bamieh commented Aug 13, 2021

@elasticmachine merge upstream

@alisonelizabeth alisonelizabeth requested a review from tobio August 13, 2021 12:33
@alisonelizabeth
Copy link
Contributor Author

Waiting to merge until @tobio can take a look and verify this doesn't break anything on the cloud side.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@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
upgradeAssistant 159.8KB 159.8KB +9.0B
Unknown metric groups

API count

id before after diff
core 2442 2444 +2

API count missing comments

id before after diff
core 1165 1166 +1

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Upgrade Assistant 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 t// v7.15.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Upgrade Assistant] Create new status API

6 participants