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

Changes tag saved object namespaceType to multiple-isolated #106341

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Jul 20, 2021

Resolves #102870
We potentially want to make the tag type shareable in the (near?) future, we need to change its namespaceType to multiple-isolated before 8.0.

A more pressing priority is that other saved objects (dashboard, visualizations, maps and lens) can reference tags, and all SO's that can be referenced need to be converted too.

This PR only changes the namespaceType from single to multiple-isolated.

Making tags "share-capable" (changing the namespaceType) will regenerate IDs of existing saved objects to ensure that they are unique across all spaces, which is a breaking change.

Checklist

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Notes
Tag ids are stored in a Kibana URL—bookmarks to legacy tags will break in non-default Kibana Space. Low (see #102870#issuecomment-883490811 for more details) High (bad UX with navigation) Should we uncover additional requirements to resolve legacy tags and aliases, we can address further changes.

For maintainers

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers marked this pull request as ready for review July 22, 2021 00:23
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner July 22, 2021 00:23
@joshdover
Copy link
Contributor

@jportner Should this be backported to 7.x? I'm guessing the convertToMultiNamespaceTypeVersion makes it safe to backport.

I wonder if we should change the release note on this PR to clarify that this is only a breaking change for URLs if you have custom plugins that use tags IDs in the URLs (since no 1st party code uses tag IDs in the URL). We should also add a note about tag IDs changing on the SO API. Maybe there's a "template" of sorts we should create for the SO API release notes for each type since the guidance would largely be the same for each type that is affected by this conversion. That, or we should just have a single release note for the SO API ID changes that mentions all affected types in 8.0.

@jportner
Copy link
Contributor

@jportner Should this be backported to 7.x? I'm guessing the convertToMultiNamespaceTypeVersion makes it safe to backport.

No, we have validation in place specifically to prevent convertToMultiNamespaceTypeVersion from being set to < 8.0.
convertToMultiNamespaceTypeVersion is what Core needs to know to automatically generate the correct "transform" function during index migrations.

namespaceType should only be changed to 'multiple-isolated' in the actual version matching convertToMultiNamespaceTypeVersion. I appreciate now how the naming doesn't make that very clear!

I wonder if we should change the release note on this PR to clarify that this is only a breaking change for URLs if you have custom plugins that use tags IDs in the URLs (since no 1st party code uses tag IDs in the URL). We should also add a note about tag IDs changing on the SO API. Maybe there's a "template" of sorts we should create for the SO API release notes for each type since the guidance would largely be the same for each type that is affected by this conversion. That, or we should just have a single release note for the SO API ID changes that mentions all affected types in 8.0.

I vote for a single release note. We have dozens of affected types, I don't think such repetitive release notes will be helpful.

@TinaHeiligers
Copy link
Contributor Author

On hold pending decision on when to change all SO types to become share capable.

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@rudolf
Copy link
Contributor

rudolf commented Aug 9, 2021

@TinaHeiligers Joe will do a single release note for the breaking change across all saved object types, but each team is responsible for converting their own types, there won't be a single big commit, so we should go ahead and merge this.

@rudolf rudolf removed the blocked label Aug 9, 2021
@jportner
Copy link
Contributor

jportner commented Aug 9, 2021

@TinaHeiligers Joe will do a single release note for the breaking change across all saved object types, but each team is responsible for converting their own types, there won't be a single big commit, so we should go ahead and merge this.

Agree that we should just do a single release note.

We were waiting on this as we had some open discussions about potentially delaying the conversion past 8.0, but it looks like we hopefully won't need to do that after all. So I think we can merge this and revert it later if we absolutely have to.

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Aug 9, 2021

So I think we can merge this and revert it later if we absolutely have to

@jportner Thanks for the head's up! I'll merge once CI passes. What should I do with the release note comment and label? Adding release_note:skip.

@TinaHeiligers
Copy link
Contributor Author

@elasticmachine merge upstream

@TinaHeiligers TinaHeiligers added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:breaking labels Aug 9, 2021
@TinaHeiligers TinaHeiligers enabled auto-merge (squash) August 9, 2021 14:46
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@TinaHeiligers TinaHeiligers merged commit fe0fb4e into elastic:master Aug 9, 2021
@TinaHeiligers TinaHeiligers deleted the so-tagging/change-namespaceType-to-multiple-isolated branch August 9, 2021 17:13
@TinaHeiligers
Copy link
Contributor Author

@jportner The PR is merged.

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

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 106341 or prevent reminders by adding the backport:skip label.

@jportner jportner added the backport:skip This commit does not require backporting label Aug 11, 2021
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Saved Object Tagging Saved Objects Tagging feature release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SO Tagging] Migrate the tag type to namespaceType : "multiple-isolated" mode
6 participants