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.

Sign up for GitHub

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

Jump to bottom

Enhancement: Design better module dependency handling #852

Merged
merged 6 commits into from
Nov 16, 2023
Merged

Enhancement: Design better module dependency handling #852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d12dac5
enhancement: Create independent module dependency handling
maryamkhidir Nov 2, 2023
e4ff7b5
Change module Pipelines to Datapipelines as declared in the config file
maryamkhidir Nov 2, 2023
da1d58e
Merge branch 'main' into enhancement/module-deps-handling
dlpzx Nov 14, 2023
531b20b
Add notifications frontend index
dlpzx Nov 14, 2023
5f85e27
Modifying logic of moduleUtils. read from index files. Add v2.1 changes
dlpzx Nov 14, 2023
c029382
Remove console.log
dlpzx Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/src/design/components/layout/DefaultSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export const DefaultSidebar = ({ openDrawer, onOpenDrawerChange }) => {
title: 'Pipelines',
path: '/console/pipelines',
icon: <BsIcons.BsGear size={15} />,
active: isModuleEnabled(ModuleNames.PIPELINES)
active: isModuleEnabled(ModuleNames.DATAPIPELINES)
};

const organizationsSection = {
Expand Down
13 changes: 13 additions & 0 deletions frontend/src/modules/Catalog/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { getModuleActiveStatus, ModuleNames } from 'utils';

export const CatalogsModule = {
moduleDefinition: true,
dlpzx marked this conversation as resolved.
Show resolved Hide resolved
name: 'catalog',
isEnvironmentModule: false,
resolve_dependency: () => {
return (
getModuleActiveStatus(ModuleNames.DATASETS) ||
getModuleActiveStatus(ModuleNames.DASHBOARDS)
);
}
};
5 changes: 5 additions & 0 deletions frontend/src/modules/Dashboards/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const DashboardsModule = {
moduleDefinition: true,
name: 'dashboards',
isEnvironmentModule: true
};
5 changes: 5 additions & 0 deletions frontend/src/modules/Datasets/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const DatasetsModule = {
moduleDefinition: true,
name: 'datasets',
isEnvironmentModule: false
dlpzx marked this conversation as resolved.
Show resolved Hide resolved
};
2 changes: 1 addition & 1 deletion frontend/src/modules/Environments/components/EnvironmentFeatures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const EnvironmentFeatures = (props) => {
{
title: 'Pipelines',
enabledEnvVariableName: 'pipelinesEnabled',
active: isModuleEnabled(ModuleNames.PIPELINES)
active: isModuleEnabled(ModuleNames.DATAPIPELINES)
}
];

Expand Down
6 changes: 3 additions & 3 deletions frontend/src/modules/Environments/views/EnvironmentCreateForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import {
} from '../services';
import {
AwsRegions,
isAnyFeatureModuleEnabled,
isAnyEnvironmentModuleEnabled,
isModuleEnabled,
ModuleNames
} from 'utils';
Expand Down Expand Up @@ -604,7 +604,7 @@ const EnvironmentCreateForm = (props) => {
</CardContent>
</Card>
<Box sx={{ mt: 3 }}>
{isAnyFeatureModuleEnabled() && (
{isAnyEnvironmentModuleEnabled() && (
<Card>
<CardHeader title="Features management" />
<CardContent>
Expand Down Expand Up @@ -710,7 +710,7 @@ const EnvironmentCreateForm = (props) => {
</FormGroup>
</Box>
)}
{isModuleEnabled(ModuleNames.PIPELINES) && (
{isModuleEnabled(ModuleNames.DATAPIPELINES) && (
<Box sx={{ ml: 2 }}>
<FormGroup>
<FormControlLabel
Expand Down
11 changes: 7 additions & 4 deletions frontend/src/modules/Environments/views/EnvironmentEditForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ import {
import { SET_ERROR, useDispatch } from 'globalErrors';
import { useClient } from 'services';
import { getEnvironment, updateEnvironment } from '../services';
import { isAnyFeatureModuleEnabled, isModuleEnabled, ModuleNames } from 'utils';
import {
isAnyEnvironmentModuleEnabled,
isModuleEnabled,
ModuleNames
} from 'utils';

const EnvironmentEditForm = (props) => {
const dispatch = useDispatch();
Expand Down Expand Up @@ -379,11 +383,10 @@ const EnvironmentEditForm = (props) => {
</CardContent>
</Card>
</Box>
{isAnyFeatureModuleEnabled() && (
{isAnyEnvironmentModuleEnabled() && (
<Box sx={{ mt: 3 }}>
<Card>
<CardHeader title="Features management" />

<CardContent>
{isModuleEnabled(ModuleNames.DASHBOARDS) && (
<Box sx={{ ml: 2 }}>
Expand Down Expand Up @@ -494,7 +497,7 @@ const EnvironmentEditForm = (props) => {
</FormGroup>
</Box>
)}
{isModuleEnabled(ModuleNames.PIPELINES) && (
{isModuleEnabled(ModuleNames.DATAPIPELINES) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Our next step (in other PR) can be extracting all the code where we check isModuleEnabled(module) into separate files. It should improve the code maintenance and reduce number of merge conflicts. I can imagine if a customer does some customization on pipelines and has to rebase and merge conflicts with a new dataset changes from main since everything in the same file :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure I understood you, you mean as components created in the module folder that are imported into the environment view? My idea for this view was to define this particular block in a more abstract way. Instead of hardcoding the different environment modules, I would prefer to have some sort of logic that creates the blocks based on the information in the index files + config activation status of the modules. But looking at it now i think we can combine both approaches:

  • iterate over the environment modules and call isModuleEnabled
  • import the correspondent environment-module-component inside the condition of isModuleEnabled

Copy link
Contributor

@dlpzx dlpzx Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We can create a new issue for this particular problem + review if we can move some of the current code from frontend/services into frontend/modules/MODULENAME/services given that we define the dependencies well. Based on feedback from customers, I will also propose to create another directory frontend/core/ where we place modules such as Environment. Wdyt? Does this make sense or is it against frontend design rules?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hardcoding the different environment modules, I would prefer to have some sort of logic that creates the blocks based on the information in the index files

This is a good idea.

I will also propose to create another directory frontend/core/ where we place modules such as Environment. Wdyt? Does this make sense or is it against frontend design rules?

I believe it's a good idea to consider which worth it's own ticket. I'd like that we gather prons and cons for it ( + we could try to start writing ADR as we had discussed before)

<Box sx={{ ml: 2 }}>
<FormGroup>
<FormControlLabel
Expand Down
13 changes: 13 additions & 0 deletions frontend/src/modules/Glossaries/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { getModuleActiveStatus, ModuleNames } from 'utils';

export const GlossariesModule = {
moduleDefinition: true,
name: 'glossaries',
isEnvironmentModule: false,
resolve_dependency: () => {
return (
getModuleActiveStatus(ModuleNames.DATASETS) ||
getModuleActiveStatus(ModuleNames.DASHBOARDS)
);
}
};
5 changes: 5 additions & 0 deletions frontend/src/modules/MLStudio/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const MLStudioModule = {
moduleDefinition: true,
name: 'mlstudio',
isEnvironmentModule: true
};
5 changes: 5 additions & 0 deletions frontend/src/modules/Notebooks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const NotebooksModule = {
moduleDefinition: true,
name: 'notebooks',
isEnvironmentModule: true
};
10 changes: 10 additions & 0 deletions frontend/src/modules/Notifications/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { getModuleActiveStatus, ModuleNames } from 'utils';

export const NotificationsModule = {
moduleDefinition: true,
name: 'notifications',
isEnvironmentModule: false,
resolve_dependency: () => {
return getModuleActiveStatus(ModuleNames.DATASETS);
dlpzx marked this conversation as resolved.
Show resolved Hide resolved
}
};
5 changes: 5 additions & 0 deletions frontend/src/modules/Pipelines/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const DataPipelinesModule = {
moduleDefinition: true,
name: 'datapipelines',
isEnvironmentModule: true
};
11 changes: 11 additions & 0 deletions frontend/src/modules/Shares/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,12 @@
import { ModuleNames, getModuleActiveStatus } from 'utils';

export const SharesModule = {
moduleDefinition: true,
name: 'shares',
isEnvironmentModule: false,
resolve_dependency: () => {
return getModuleActiveStatus(ModuleNames.DATASETS);
}
};

export { ShareInboxList } from './components';
13 changes: 13 additions & 0 deletions frontend/src/modules/Worksheets/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { getModuleActiveStatus, ModuleNames } from 'utils';

export const WorksheetsModule = {
moduleDefinition: true,
name: 'worksheets',
isEnvironmentModule: false,
resolve_dependency: () => {
return (
getModuleActiveStatus(ModuleNames.DATASETS) &&
getModuleActiveStatus(ModuleNames.WORKSHEETS)
);
}
};
10 changes: 10 additions & 0 deletions frontend/src/modules/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export * from './Catalog';
export * from './Dashboards';
export * from './Datasets';
export * from './Glossaries';
export * from './MLStudio';
export * from './Notebooks';
export * from './Notifications';
export * from './Pipelines';
export * from './Shares';
export * from './Worksheets';
2 changes: 1 addition & 1 deletion frontend/src/routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ const routes = [
}
]
},
isModuleEnabled(ModuleNames.PIPELINES) && {
isModuleEnabled(ModuleNames.DATAPIPELINES) && {
children: [
{
path: 'pipelines',
Expand Down
73 changes: 35 additions & 38 deletions frontend/src/utils/helpers/moduleUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,24 @@
/* eslint-disable no-restricted-properties */
import * as modules from 'modules';
import config from '../../generated/config.json';

const ModuleNames = {
CATALOG: 'catalog',
DATASETS: 'datasets',
SHARES: 'shares',
GLOSSARIES: 'glossaries',
WORKSHEETS: 'worksheets',
NOTEBOOKS: 'notebooks',
MLSTUDIO: 'mlstudio',
PIPELINES: 'datapipelines',
DASHBOARDS: 'dashboards',
NOTIFICATIONS: 'notifications'
};
function _resolveModuleName(module) {
return Object.values(modules).find((_module) => _module.name === module);
}

function _hasDependencyModule(module) {
const resolvedModule = _resolveModuleName(module);
return typeof resolvedModule?.resolve_dependency === 'function';
}

function isModuleEnabled(module) {
if (module === ModuleNames.CATALOG || module === ModuleNames.GLOSSARIES) {
return (
getModuleActiveStatus(ModuleNames.DATASETS) ||
getModuleActiveStatus(ModuleNames.DASHBOARDS)
);
if (_hasDependencyModule(module)) {
const resolvedModule = _resolveModuleName(module);
return resolvedModule.resolve_dependency();
}
if (module === ModuleNames.SHARES || module === ModuleNames.NOTIFICATIONS) {
return getModuleActiveStatus(ModuleNames.DATASETS);
}
if (module === ModuleNames.WORKSHEETS) {
return (
getModuleActiveStatus(ModuleNames.DATASETS) &&
getModuleActiveStatus(ModuleNames.WORKSHEETS)
);
}

return getModuleActiveStatus(module);
}

function isAnyFeatureModuleEnabled() {
return !!(
isModuleEnabled(ModuleNames.PIPELINES) ||
isModuleEnabled(ModuleNames.DASHBOARDS) ||
isModuleEnabled(ModuleNames.MLSTUDIO) ||
isModuleEnabled(ModuleNames.NOTEBOOKS)
);
}

function getModuleActiveStatus(moduleKey) {
if (
config.modules &&
Expand Down Expand Up @@ -71,9 +47,30 @@ function isFeatureEnabled(moduleKey, featureKey) {
return false;
}

function isAnyEnvironmentModuleEnabled() {
const env_modules = Object.values(modules).filter(
(_module) =>
_module.isEnvironmentModule === true && isModuleEnabled(_module.name)
);
return env_modules.length > 0 ? true : false;
}

function _modulesNameMap() {
const map = {};
for (const module of Object.values(modules).filter(
(_module) => _module.moduleDefinition === true
)) {
const upperCaseModule = module.name.toUpperCase();
map[upperCaseModule] = module.name;
}
return map;
}

const ModuleNames = _modulesNameMap();
export {
ModuleNames,
isModuleEnabled,
isAnyFeatureModuleEnabled,
isFeatureEnabled
getModuleActiveStatus,
isFeatureEnabled,
isAnyEnvironmentModuleEnabled
};
Loading