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] Fix circular ref by moving code & tests into Fleet #94171

Merged
merged 3 commits into from
Mar 11, 2021
Merged

[Fleet] Fix circular ref by moving code & tests into Fleet #94171

merged 3 commits into from
Mar 11, 2021

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Mar 9, 2021 •
edited
Loading

Problem

There's a circular dependency #91111 between the fleet and security_solution plugins

  • security_solution depends on fleet, but
  • fleet has (had with this PR) an import from security_solution (migrations for the 7.11 and 7.12 package policy objects)

Proposed solution

(A) This PR

Move the two imported functions from security into fleet.

(B) Follow up issue #94480

Putting integration-specific code into fleet doesn't scale (technically or cognitively). Discuss if this use case (specifying saved object migrations, etc) applies to other plugins. e.g. can apm do this? nginx? If so, should we find a way to move this out of fleet?

Checklist

closes #91111

@jfsiii jfsiii 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.13.0 labels Mar 9, 2021
@jfsiii jfsiii self-assigned this Mar 9, 2021
@jfsiii jfsiii marked this pull request as ready for review March 10, 2021 01:32
@jfsiii jfsiii requested review from a team as code owners March 10, 2021 01:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@jfsiii
Copy link
Contributor Author

jfsiii commented Mar 10, 2021

cc @kevinlog @paul-tavares @parkiino

nchaulet
nchaulet approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

👍

@jfsiii
Copy link
Contributor Author

jfsiii commented Mar 11, 2021

@elasticmachine merge upstream

@jfsiii jfsiii added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 11, 2021
@jfsiii jfsiii enabled auto-merge (squash) March 11, 2021 17:30
@jfsiii jfsiii mentioned this pull request Mar 11, 2021
Open
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

Kibana Pipeline / general / "before all" hook for "should open a modal".Open timeline Open timeline modal "before all" hook for "should open a modal"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: Expected to find element: `[data-test-subj="title-55f30850-8299-11eb-833f-cfd11c625c42"]`, but never found it.

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `Open timeline`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at Object.openTimelineById (http://localhost:61211/__cypress/tests?p=cypress/integration/timelines/open_timeline.spec.ts:16088:15)
    at Context.eval (http://localhost:61211/__cypress/tests?p=cypress/integration/timelines/open_timeline.spec.ts:15046:28)

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2202 2200 -2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.8MB 7.8MB +4.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 237.4KB 233.6KB -3.8KB

History

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

cc @jfsiii

@jfsiii jfsiii merged commit 6491ad5 into elastic:master Mar 11, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 11, 2021
@kibanamachine
[Fleet] Fix circular ref by moving code & tests into Fleet (elastic#9…
26e1858 
…4171)

## Problem
There's a circular dependency elastic#91111 between the `fleet` and `security_solution` plugins 

* `security_solution` depends on `fleet`, but 
* `fleet` has (_had_ with this PR) an `import` from  `security_solution` (migrations for the 7.11 and 7.12 package policy objects)

## Proposed solution
### (A) This PR
Move the two imported functions from `security` into `fleet`.

### (B) Follow up issue
Putting integration-specific code into `fleet` doesn't scale (technically or cognitively). Discuss if this use case (specifying saved object migrations, etc) applies to other plugins. e.g. can `apm` do this? `nginx`? If so, should we find a way to move this out of `fleet`?


### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

closes elastic#91111
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Mar 11, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.x / #94483

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 11, 2021
@kibanamachine
[Fleet] Fix circular ref by moving code & tests into Fleet (#94171) (#…
e0094d3 
…94483)

## Problem
There's a circular dependency #91111 between the `fleet` and `security_solution` plugins 

* `security_solution` depends on `fleet`, but 
* `fleet` has (_had_ with this PR) an `import` from  `security_solution` (migrations for the 7.11 and 7.12 package policy objects)

## Proposed solution
### (A) This PR
Move the two imported functions from `security` into `fleet`.

### (B) Follow up issue
Putting integration-specific code into `fleet` doesn't scale (technically or cognitively). Discuss if this use case (specifying saved object migrations, etc) applies to other plugins. e.g. can `apm` do this? `nginx`? If so, should we find a way to move this out of `fleet`?


### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

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

Co-authored-by: John Schulz <john.schulz@elastic.co>
@jfsiii jfsiii deleted the 91111-remove-fleet-security-circular-dependency branch April 6, 2021 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jen-huang jen-huang jen-huang left review comments

@kevinlog kevinlog kevinlog approved these changes

@nchaulet nchaulet nchaulet approved these changes

Assignees

@jfsiii jfsiii

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.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove circular dependency between fleet and security_solution
6 participants
@jfsiii @elasticmachine @kibanamachine @nchaulet @jen-huang @kevinlog