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.

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

Remove legacy spaces plugin #74532

Merged
merged 6 commits into from
Sep 18, 2020
Merged

Conversation

legrego
Copy link
Member

@legrego legrego commented Aug 6, 2020

Summary

Removes the legacy spaces plugin 🎉

Blocked on #41983

@legrego legrego added chore Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.10.0 labels Aug 6, 2020
@legrego legrego requested a review from a team as a code owner August 6, 2020 14:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@legrego
Copy link
Member Author

legrego commented Aug 6, 2020

@spalger tagging you since I remember you worked on this about a year ago. This PR is failing because @kbn/es-archiver relies on the legacy status api to figure out of spaces is enabled, and removing the legacy spaces plugin causes these checks to return false instead of true:

if (!kibanaPluginIds.includes('spaces')) {

I know the status API hasn't been migrated yet, but are you aware of anything else we can use in the interim to make this determination?

@spalger
Copy link
Contributor

spalger commented Aug 6, 2020

Well, we could try always behaving like spaces are enabled, but I would rather not make changes to that logic without more careful consideration and there is a lot to consider... I would prefer that we finish the status API migration first but I'm not sure what the timeline is on that. @elastic/kibana-platform?

@joshdover
Copy link
Contributor

I would prefer that we finish the status API migration first but I'm not sure what the timeline is on that. @elastic/kibana-platform?

It's our roadmap for 7.10. I may be able to focus on that next week, but may also be pulled into some other unrelated scaling tasks. I think we're ok to leave this PR open for a while and we can merge it once #41983 is completed.

@legrego legrego added the blocked label Aug 6, 2020
@legrego legrego marked this pull request as draft August 10, 2020 11:05
@joshdover joshdover mentioned this pull request Aug 24, 2020
14 tasks
@pgayvallet pgayvallet mentioned this pull request Sep 16, 2020
3 tasks
@legrego legrego marked this pull request as ready for review September 17, 2020 14:41
@legrego legrego removed the blocked label Sep 17, 2020
@legrego
Copy link
Member Author

legrego commented Sep 17, 2020

Test flakiness appears unrelated to this PR

Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

I found a couple other references to "legacy/plugins/spaces" in x-pack/tsconfig.json, x-pack/.i18nrc.json, .github/CODEOWNERS, and src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json, could you remove those too?

@legrego
Copy link
Member Author

legrego commented Sep 17, 2020

I found a couple other references to "legacy/plugins/spaces" in x-pack/tsconfig.json, x-pack/.i18nrc.json, .github/CODEOWNERS, and src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json, could you remove those too?

Wow, thanks! My code searching skills are not up to par lately

@legrego legrego requested a review from a team as a code owner September 17, 2020 19:22
Copy link
Contributor

@jportner jportner left a comment

Choose a reason for hiding this comment

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

LGTM! 😀

@wayneseymour
Copy link
Member

src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json will be removed soon, fyi :)

@legrego
Copy link
Member Author

legrego commented Sep 17, 2020

src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json will be removed soon, fyi :)

@wayneseymour that's great! Are you suggesting that I revert the change since it's going away, or is the change OK from your perspective?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

distributable file count

id value diff baseline
default 45919 -2 45921

History

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

@wayneseymour
Copy link
Member

src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json will be removed soon, fyi :)

@wayneseymour that's great! Are you suggesting that I revert the change since it's going away, or is the change OK from your perspective?

Honestly, either way is fine, imho. @LeeDr ?

@legrego
Copy link
Member Author

legrego commented Sep 18, 2020

Since it's not impacting CI from what I can tell, I'll merge as-is to unblock the Platform team. Let me know if you'd like that reverted and I'll be happy to do so.

@legrego legrego merged commit 32a8eb2 into elastic:master Sep 18, 2020
@legrego legrego deleted the spaces/remove-legacy-plugin branch September 18, 2020 14:15
legrego added a commit to legrego/kibana that referenced this pull request Sep 18, 2020
# Conflicts:
#	.github/CODEOWNERS
#	src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json
legrego added a commit that referenced this pull request Sep 18, 2020
# Conflicts:
#	.github/CODEOWNERS
#	src/dev/code_coverage/ingest_coverage/team_assignment/ingestion_pipeline_painless.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants