-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Refractor] Feature flag #102
Conversation
Codecov Report
@@ Coverage Diff @@
## workspace #102 +/- ##
==============================================
- Coverage 65.75% 54.72% -11.03%
==============================================
Files 3336 3038 -298
Lines 64493 60368 -4125
Branches 10264 9632 -632
==============================================
- Hits 42406 33038 -9368
- Misses 19541 25211 +5670
+ Partials 2546 2119 -427
Flags with carried forward coverage won't be shown. Click here to find out more. see 702 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
path: `${WORKSPACES_API_BASE_URL}/is_workspace_enabled`, | ||
validate: {}, | ||
}, | ||
router.handleLegacyErrors(async (context, req, res) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it call context.core.uiSettings.client.get(FEATURE_FLAG_KEY_IN_UI_SETTING)
to get the flag directly from saved objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the flag in browser side should be consistent to the flag in server side, but not the opensearch side. The flag in server side may not be consistent to the flag in kibana index because user may call index update API to update the flag directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so if changing the flag in the index but not via setWorkspaceFeatureFlag
will require a server restart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and that is not a expected behavior to update flag.
private config$: Observable<ConfigSchema>; | ||
private enabled$: BehaviorSubject<boolean> = new BehaviorSubject(false); | ||
private loopRequestTimer?: NodeJS.Timeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my fault. There was a timer to check the flag in kibana index and try to keep the enabled$ consistent with the real flag in kibana index. Done for removing this.
b27bcb4
to
04814a3
Compare
@@ -396,7 +396,7 @@ export class WorkspaceSavedObjectsClientWrapper { | |||
|
|||
const isDashboardAdmin = this.isDashboardAdmin(wrapperOptions.request); | |||
|
|||
if (isDashboardAdmin) { | |||
if (isDashboardAdmin || !this.options.enabled$.getValue()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The #scopedSavedObjectsClient
in src/core/server/core_route_handler_context.ts
won't be re-initialized which means will always use the wrapperOptions.client
after enabled$
changed. Is this could be an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, did not know that but it is fine because savedObjectsClient does not rely on enabled$.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the client wrapper won't work as expect after enabled change. Such as we can test if the ACL check will be by passed after enabled change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, when we enable the feature flag, the client is generated with internalRepository
, and after we switch the flag, the savedObjectsClient will still be internalSavedObjectsClient
, is this what you want to point out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wanglam I did some changes to generate savedObjectsClient in runtime, please help to take a look. It should fix the client cache problem.
91605cc
to
53dfc5c
Compare
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
4c9e05b
to
11ce883
Compare
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: add some flag Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * refractor: add enabled$ in server side Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add logic check in public side Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feature: remove useless property Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: some modify * feat: merge Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: add some flag Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * refractor: add enabled$ in server side Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add logic check in public side Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feature: remove useless property Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: some modify * feat: merge Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
* feat: add some flag Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * refractor: add enabled$ in server side Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add logic check in public side Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feature: remove useless property Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: some modify * feat: merge Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Description
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr