Skip to content

Conversation

paula-stacho
Copy link
Collaborator

@paula-stacho paula-stacho commented Nov 28, 2024

Description

Although this flag was created, it was never actually used. It was also defined in InternalUserPreferences instead of FeatureFlags. How did this work before? It was because atlasMetadata is missing for Electron Compass, so instead of the flag the isGlobalWritesSupported condition was never met:

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the fix label Nov 28, 2024
@paula-stacho paula-stacho added the no release notes Fix or feature not for release notes label Nov 28, 2024
@paula-stacho paula-stacho marked this pull request as ready for review November 28, 2024 15:22
enableQueryHistoryAutocomplete: boolean;
enableProxySupport: boolean;
enableRollingIndexes: boolean;
enableGlobalWrites: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we adding it as a feature flag on compass side? should it not be part of preferences and FF as part of MMS only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. Tbh I am confused about the different types of preferences in compass and wanted to check this with @gribnoysup. I think Global Writes should be in the same group as Rolling Indexes, as both features are web only for now, but we might be adding them to desktop later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, so I think it's understandable that you can see these in either of those preferences groups, we don't really have a very clean separation there for case like this.

Saying that I think these map more to what we use the feature flag group of preferences for: they are temporary, will only be used for the initial rollout and then removed, we don't really need a dedicated preference for these features as their availability in UI has a stricter check against a cluster metadata that is not available in compass desktop (and at the point where it will be, they should start working without the feature flag required to toggle them)

This is different from something like saved queries feature where bringing it to the web will require more effort in making the system work universally across environments and so it's not something that is just temporarily disabled in the same sense as these that will be used for the rollout.

And to be clear this is a very loose definition that can be understandably argued against, we might consider introducing some new preferences group to remove the ambiguity, maybe something to think through a bit more, but probably doesn't need to block this particular change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

my only concern was if this is a ff on compass side, it will show up in settings-modal. but not a blocker.

@mabaasit
Copy link
Collaborator

thanks for catching this. I added it in InternalUserPreferences initially because FeatureFlags are specific to Compass and we want CompassWeb to pass this as a preference. And for MMS, its a feature flag, right?

@paula-stacho paula-stacho changed the title fix: global writes feture flag COMPAS-8524 fix: global writes feature flag COMPAS-8524 Nov 28, 2024
@paula-stacho paula-stacho merged commit 36a5c11 into main Nov 29, 2024
35 of 37 checks passed
@paula-stacho paula-stacho deleted the COMPASS-8524-compass branch November 29, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix no release notes Fix or feature not for release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants