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

[Core][Deprecations] omit deprecationDetails if needed it in the reques #114399

Merged
merged 6 commits into from
Oct 11, 2021
Merged

[Core][Deprecations] omit deprecationDetails if needed it in the reques #114399

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7a1415
omit deprecationDetails if needed it
XavierM Oct 8, 2021
e80d967
review I
XavierM Oct 11, 2021
8169153
doc update
XavierM Oct 11, 2021
1218ab3
Merge branch 'master' of github.com:elastic/kibana into omit_deprecat…
XavierM Oct 11, 2021
0d09309
Merge branch 'master' into omit_deprecation_details
kibanamachine Oct 11, 2021
e729918
Merge branch 'master' into omit_deprecation_details
kibanamachine Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/core/public/deprecations/deprecations_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,5 +197,38 @@ describe('DeprecationsClient', () => {

expect(result).toEqual({ status: 'fail', reason: mockResponse });
});

it('omit deprecationDetails in the request of the body', async () => {
const deprecationsClient = new DeprecationsClient({ http });
const mockDeprecationDetails: DomainDeprecationDetails = {
title: 'some-title',
domainId: 'testPluginId-1',
message: 'some-message',
level: 'warning',
correctiveActions: {
api: {
path: 'some-path',
method: 'POST',
body: {
extra_param: 123,
},
},
manualSteps: ['manual-step'],
},
omitDeprecationDetails: true,
};
const result = await deprecationsClient.resolveDeprecation(mockDeprecationDetails);

expect(http.fetch).toBeCalledTimes(1);
expect(http.fetch).toBeCalledWith({
path: 'some-path',
method: 'POST',
asSystemRequest: true,
body: JSON.stringify({
extra_param: 123,
}),
});
expect(result).toEqual({ status: 'ok' });
});
});
});
4 changes: 2 additions & 2 deletions src/core/public/deprecations/deprecations_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class DeprecationsClient {
public resolveDeprecation = async (
details: DomainDeprecationDetails
): Promise<ResolveDeprecationResponse> => {
const { domainId, correctiveActions } = details;
const { domainId, correctiveActions, omitDeprecationDetails = false } = details;
// explicit check required for TS type guard
if (typeof correctiveActions.api !== 'object') {
return {
Expand All @@ -67,7 +67,7 @@ export class DeprecationsClient {
asSystemRequest: true,
body: JSON.stringify({
...body,
deprecationDetails: { domainId },
...(omitDeprecationDetails ? {} : { deprecationDetails: { domainId } }),
}),
Copy link
Contributor

@pgayvallet pgayvallet Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@Bamieh do you know why we're adding this in the first place? I suspect more endpoints may fail to validate because of this added parameter.

  • Are we documenting this anywhere?
  • Shouldn't we just remove this behavior? What is it supposed to be used for?

Copy link
Member

@Bamieh Bamieh Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The resolve API endpoint has no way of telling which deprecation domain ID the user is attempting to resolve (via UA or a direct endpoint call) unless each deprecation explicitly passed the domainId in the correctiveAction.api.body.

Is this documented?
I do mention that we pass the api.body and the context of the deprecation in the docs

The field is shown in the example route in the functional tests

Shouldn't we just remove this behavior? What is it supposed to be used for?

the domainId is the id of the plugin and it is automatically passed to each deprecation. I am passing it to the api body as well to avoid plugins requirinig to manually pass their domainId to the api if they need it.

Looking at the current depreaction routes across kibana I don't see this used yet so we can remove it completely. I am curious why not just add this object to the schema of your routes @XavierM rather than omitting it by adding extra logic.

Copy link
Contributor

@pgayvallet pgayvallet Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The resolve API endpoint has no way of telling which deprecation domain ID the user is attempting to resolve

Unless I understand

kibana/src/core/public/deprecations/deprecations_client.ts

Lines 48 to 50 in 249c5fb

public resolveDeprecation = async (
details: DomainDeprecationDetails
): Promise<ResolveDeprecationResponse> => {

wrong, we're performing a call against the endpoint registered via DomainDeprecationDetails.correctiveActions.api. as all these info are registered by the deprecation owners, I would expect them to be self-sufficient to resolve the deprecation?

I am passing it to the api body as well to avoid plugins requirinig to manually pass their domainId to the api if they need it

Sorry, to which API? do automatic resolution endpoints need to call a deprecation API somehow after the resolution?

Copy link
Member

Choose a reason for hiding this comment

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

Here is an example how a deprecation is registered:

kibana/x-pack/plugins/reporting/server/deprecations/migrate_existing_indices_ilm_policy.ts

Lines 32 to 60 in 249c5fb

{
title: i18n.translate('xpack.reporting.deprecations.migrateIndexIlmPolicyActionTitle', {
defaultMessage: 'Found reporting indices managed by custom ILM policy.',
}),
level: 'warning',
message: i18n.translate('xpack.reporting.deprecations.migrateIndexIlmPolicyActionMessage', {
defaultMessage: `New reporting indices will be managed by the "{reportingIlmPolicy}" provisioned ILM policy. You must edit this policy to manage the report lifecycle. This change targets all indices prefixed with "{indexPattern}".`,
values: {
reportingIlmPolicy: ILM_POLICY_NAME,
indexPattern,
},
}),
correctiveActions: {
manualSteps: [
i18n.translate(
'xpack.reporting.deprecations.migrateIndexIlmPolicy.manualStepOneMessage',
{
defaultMessage:
'Update all reporting indices to use the "{reportingIlmPolicy}" policy using the index settings API.',
values: { reportingIlmPolicy: ILM_POLICY_NAME },
}
),
],
api: {
method: 'PUT',
path: API_MIGRATE_ILM_POLICY_URL,
},
},
},

The domainId is not passed by the plugins. It is automatically attached to the registered deprecation based on which plugin is doing this registration.

The plugins register their own routes to correct the deprecation, and since i'm providing the domainId during deprecation registration, i thought i'd also pass it to the route as well.

The correctiveAction.api is called by the deprecations service DeprecationsClient which is used by the UA

Copy link
Member

Choose a reason for hiding this comment

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

I had a zoom call with @XavierM and this approach of adding a flag LGTM for now. They are not creating their own route for resolving the deprecation so it is not possible to just pass this provided context object and allowing it in the route schema.

I think it is fine to have this flag inside correctiveActions.api.omitContextFromBody. We can revisit this in 8.x (if needed) since we can change it on a more comfortable schedule and we'll have less custom routes to resolve deprecations.

});
return { status: 'ok' };
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/deprecations/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ export interface DeprecationsDetails {
*/
manualSteps: string[];
};
/* Allow to omit the attribute "deprecationDetails" in the request of the body */
omitDeprecationDetails?: boolean;
}

/**
Expand Down