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

Jump to bottom

[Fleet] Include ID field in packages on get/create #100908

Merged
merged 2 commits into from
Jun 1, 2021
Merged

[Fleet] Include ID field in packages on get/create #100908

merged 2 commits into from
Jun 1, 2021

Conversation

kpollich
Copy link
Member

Add id field for packages in info/create requests

Closes #100897

Fixes error in E2E tests reported here: #99866 (comment)

@kpollich kpollich added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 28, 2021
@kpollich kpollich self-assigned this May 28, 2021
@kpollich kpollich requested a review from a team as a code owner May 28, 2021 15:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@mdelapenya
Copy link
Contributor

/run-fleet-e2e-tests

nchaulet
nchaulet reviewed May 28, 2021
View reviewed changes
x-pack/plugins/fleet/server/types/models/package_policy.ts Outdated
@@ -36,6 +36,7 @@ const PackagePolicyBaseSchema = {
enabled: schema.boolean(),
package: schema.maybe(
schema.object({
id: schema.string(),
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 have a schema.maybe() for retro compatibility ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes good call.

kpollich
kpollich commented May 28, 2021
View reviewed changes
x-pack/plugins/fleet/server/services/epm/packages/get.ts Outdated
@@ -126,6 +126,7 @@ export async function getPackageInfo(options: {
title: packageInfo.title || nameAsTitle(packageInfo.name),
assets: Registry.groupPathsByService(paths || []),
removable: !isRequiredPackage(pkgName),
id: packageInfo.name,
Copy link
Member Author

Choose a reason for hiding this comment

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

This mimics the behavior in the list endpoint: https://github.com/elastic/kibana/blob/master/x-pack/plugins/fleet/server/services/epm/packages/get.ts#L51

x-pack/plugins/fleet/server/saved_objects/index.ts Outdated
@@ -224,6 +224,7 @@ const getSavedObjectTypes = (
output_id: { type: 'keyword' },
package: {
properties: {
id: { type: 'keyword' },
Copy link
Member Author

Choose a reason for hiding this comment

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

Does touching these saved object mappings require anything else? Is this what migrations are for? We don't necessarily have to persist this ID, and we could instead just implement a workaround to allow it at the request/response level.

@kpollich
Allow for ID in create package policy request
9fb470f 
E2E tests are failing because they include the ID field returned by the
package list endpoint. This just updates our request schema to accept an
ID, though we don't persist or deal with the ID anywhere.

Closes elastic#100897
@kpollich kpollich force-pushed the 100897-allow-id-for-package-policy-create branch from 61bc055 to 9fb470f Compare May 28, 2021 17:43
@kpollich
Copy link
Member Author

After tracking down a bunch of type issues where we reference package objects, it seemed better to just fix the immediate issue with the E2E tests instead, so I've updated the approach in this PR to just avoid erroring when an ID is provided in the create package policy request body.

@kpollich
Copy link
Member Author

/run-fleet-e2e-tests

@kpollich
Copy link
Member Author

@mdelapenya I kicked off the E2E tests and it seems like they're still failing. I'm seeing 22 failures on https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/master/927/tests - maybe I'm looking in the wrong place or misunderstanding?

@mdelapenya
Copy link
Contributor

@mdelapenya I kicked off the E2E tests and it seems like they're still failing. I'm seeing 22 failures on https://beats-ci.elastic.co/blue/organizations/jenkins/e2e-tests%2Fe2e-testing-mbp/detail/master/927/tests - maybe I'm looking in the wrong place or misunderstanding?

Let me debug this

This was referenced May 29, 2021
Merged
Closed
@kpollich
Copy link
Member Author

kpollich commented Jun 1, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 390 346 -44
stackAlerts 101 95 -6
total -342

History

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

cc @kpollich

@kpollich kpollich merged commit 151f5fd into elastic:master Jun 1, 2021
@kpollich kpollich deleted the 100897-allow-id-for-package-policy-create branch June 1, 2021 13:30
@kibanamachine kibanamachine mentioned this pull request Jun 1, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 1, 2021
@kpollich @kibanamachine
Allow for ID in create package policy request (elastic#100908)
0383b87 
E2E tests are failing because they include the ID field returned by the
package list endpoint. This just updates our request schema to accept an
ID, though we don't persist or deal with the ID anywhere.

Closes elastic#100897

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit that referenced this pull request Jun 1, 2021
@kibanamachine @kpollich
Allow for ID in create package policy request (#100908) (#101053)
8a205e3 
E2E tests are failing because they include the ID field returned by the
package list endpoint. This just updates our request schema to accept an
ID, though we don't persist or deal with the ID anywhere.

Closes #100897

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kyle Pollich <kyle.pollich@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nchaulet nchaulet nchaulet approved these changes

Assignees

@kpollich kpollich

Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Fleet] Allow Package ID in Create Package Policy requests
5 participants
@kpollich @elasticmachine @mdelapenya @kibanamachine @nchaulet