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

[backend/frontend] Introduce dev flags to deactivate feature in ongoing development #5802

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

richard-julien
Copy link
Member

This PR aims to introduce a dev configuration to disable features that are currently in development but not ready yet.
That's will ease the merging of large feature but providing simple tools to hide the feature.

In this PR you will find a example of Decay indicator feature deactivation.

  • Deactivation of the manager registration
  • Deactivation of the module registration
  • Deactivation of a frontend specific menu

@richard-julien richard-julien added feature use for describing a new feature to develop architecture improvement Architecture refactor or improvement is needed labels Feb 2, 2024
@richard-julien richard-julien self-assigned this Feb 2, 2024
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 58 lines in your changes are missing coverage. Please review.

Comparison is base (ae28815) 64.82% compared to head (1864cb6) 64.75%.
Report is 6 commits behind head on master.

❗ Current head 1864cb6 differs from pull request most recent head d0b0952. Consider uploading reports for the commit d0b0952 to get more accurate results

Files Patch % Lines
...opencti-graphql/src/modules/indicator/indicator.ts 8.62% 53 Missing ⚠️
...encti-graphql/src/manager/indicatorDecayManager.ts 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5802      +/-   ##
==========================================
- Coverage   64.82%   64.75%   -0.08%     
==========================================
  Files         521      521              
  Lines       61348    61366      +18     
  Branches     4900     4905       +5     
==========================================
- Hits        39770    39737      -33     
- Misses      21578    21629      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@aHenryJard aHenryJard left a comment

Choose a reason for hiding this comment

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

I just did a PR review, I did not tried it yet.

To avoid mispelling, I think it's better to have the 'ed' everywhere (enabled, disabled, activated etc..) so I did suggestions in that way.


const CustomizationMenu: FunctionComponent = () => {
const { isFeatureEnable } = useHelper();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { isFeatureEnable } = useHelper();
const { isFeatureEnabled } = useHelper();

@@ -19,6 +21,10 @@ const CustomizationMenu: FunctionComponent = () => {
path: '/dashboard/settings/customization/retention',
label: 'Retention policies',
},
...(isFeatureEnable('INDICATOR_DECAY') ? [{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
...(isFeatureEnable('INDICATOR_DECAY') ? [{
...(isFeatureEnabled('INDICATOR_DECAY') ? [{

@@ -36,7 +36,7 @@ const isFeatureEnable = (
) => {
const flags = settings.platform_feature_flags ?? [];
const feature = flags.find((f) => f.id === id);
return feature !== undefined && feature.enable === true;
return feature === undefined || feature.enable === true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return feature === undefined || feature.enable === true;
return feature === undefined || feature.enabled === true;

label={
module.enable ? t_i18n('Enabled') : t_i18n('Disabled')
}
label={module.enable ? t_i18n('Enabled') : t_i18n('Disabled')}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
label={module.enable ? t_i18n('Enabled') : t_i18n('Disabled')}
label={module.enabled ? t_i18n('Enabled') : t_i18n('Disabled')}

@labo-flg
Copy link
Member

labo-flg commented Feb 5, 2024

Let's enable all feature flags in the backend tests, to cover the code during development even if not enabled in production yet.

@Kedae Kedae added filigran team use to identify PR from the Filigran team and removed feature use for describing a new feature to develop labels Feb 5, 2024
@richard-julien
Copy link
Member Author

richard-julien commented Feb 5, 2024

I just did a PR review, I did not tried it yet.

To avoid mispelling, I think it's better to have the 'ed' everywhere (enabled, disabled, activated etc..) so I did suggestions in that way.

That's a good point but there is more work and alignment to do than your proposal to really align everything around "enabled" like the modules that share the graphql object for the result. Modules "enable" is currently written in redis so the modification of the name must be changed carefully.

For now i will propose to only focus on the flag here and do the change of semantic in another PR

Copy link
Member

@aHenryJard aHenryJard left a comment

Choose a reason for hiding this comment

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

Tested locally on Decay feature, it's working as expected.

I tested:

  • without disabled_dev_features => decay hidden, ok
  • with disabled_dev_features empty => decay enabled, ok
  • with disabled_dev_features = ['INDICATOR_DECAY'] => decay hidden, ok

@richard-julien
Copy link
Member Author

Let's enable all feature flags in the backend tests, to cover the code during development even if not enabled in production yet.

Done

@richard-julien richard-julien merged commit af7ab66 into master Feb 5, 2024
5 of 6 checks passed
@richard-julien richard-julien deleted the oob/dev_flags branch February 5, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture improvement Architecture refactor or improvement is needed filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants