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

Remove circular dependency between features and security #100206

Merged
merged 10 commits into from
May 19, 2021
Merged

Remove circular dependency between features and security #100206

merged 10 commits into from
May 19, 2021

Conversation

legrego
Copy link
Member

@legrego legrego commented May 17, 2021 •
edited
Loading

Summary

Moves the feature_privileges_iterator out of the security plugin and into the feature plugin.

Resolves #87388

@legrego legrego added auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels May 17, 2021
@legrego legrego marked this pull request as ready for review May 17, 2021 13:59
@legrego legrego requested review from a team as code owners May 17, 2021 13:59
mshustov
mshustov reviewed May 17, 2021
View reviewed changes
x-pack/plugins/security/server/authorization/oss_features.test.ts Outdated
describe(`with a ${licenseType} license`, () => {
it(`returns the ${featureConfig.id} feature augmented with appropriate sub feature privileges`, () => {
const privileges = [];
for (const featurePrivilege of featurePrivilegeIterator(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason featurePrivilegeIterator cannot belong to features plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 I can't think of a reason why it can't belong to the features plugin. It's only required by the security plugin today, but it's also not coupled to anything else within security. If you'd prefer to make this part of the features plugin contract, then I can move this out of security

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's move, IMO it belongs to features plugin domain.

@legrego
Copy link
Member Author

legrego commented May 18, 2021

@elasticmachine merge upstream

jportner
jportner approved these changes May 18, 2021
View reviewed changes
x-pack/plugins/features/server/feature_privilege_iterator/sub_feature_privilege_iterator.ts

export function* subFeaturePrivilegeIterator(
export type SubFeaturePrivilegeIterator = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you added a TSDoc comment for FeaturePrivilegeIterator, but missed it for SubFeaturePrivilegeIterator here.

@legrego legrego enabled auto-merge (squash) May 19, 2021 13:14
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
features 1 3 +2
Unknown metric groups

API count

id before after diff
features 200 202 +2

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
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
stackAlerts 101 95 -6
total -295

History

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

@legrego legrego merged commit 4256c18 into elastic:master May 19, 2021
@kibanamachine kibanamachine mentioned this pull request May 19, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 19, 2021
@legrego @kibanamachine
Remove circular dependency between features and security (elastic#100206
f4c52a6 
)

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 21, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 21, 2021
kibanamachine added a commit that referenced this pull request May 21, 2021
@kibanamachine @legrego
Remove circular dependency between features and security (#100206) (#…
c4b91ac 
…100341)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Larry Gregory <[email protected]>
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
@legrego @kibanamachine @yctercero
Remove circular dependency between features and security (elastic#100206
d3cb326 
)

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jportner jportner jportner approved these changes

@mshustov mshustov mshustov approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the circular dependency between features and security
4 participants
@legrego @kibanamachine @mshustov @jportner