-
Notifications
You must be signed in to change notification settings - Fork 82
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
Generic dataset module and specific s3_datasets module - part 6 (Frontend) #1292
Conversation
… creation renamed table s3_dataset
…eric-dataset-model-refactoring-2
…t-model-refactoring-2' into feat/generic-dataset-model-refactoring-3
…eric-dataset-model-refactoring-3 # Conflicts: # backend/dataall/modules/dataset_sharing/services/dataset_sharing_service.py # backend/dataall/modules/s3_datasets/api/dataset/resolvers.py # backend/dataall/modules/s3_datasets/db/dataset_models.py # backend/dataall/modules/s3_datasets/services/dataset_service.py # backend/dataall/modules/s3_datasets/services/dataset_table_service.py
…irst list API for datasets_base
…to feat/generic-dataset-model-refactoring-4
…to feat/generic-dataset-model-refactoring-5
…oup and move it into Worksheets in FE
…eric-dataset-model-refactoring-6 # Conflicts: # backend/dataall/modules/datasets_base/db/dataset_repositories.py # backend/dataall/modules/datasets_base/services/dataset_list_service.py # frontend/src/modules/S3_Datasets/views/DatasetView.js
Testing locally:
|
b9d38de
to
55531b1
Compare
@@ -7,7 +7,8 @@ export const CatalogsModule = { | |||
resolve_dependency: () => { | |||
return ( | |||
getModuleActiveStatus(ModuleNames.S3_DATASETS) || | |||
getModuleActiveStatus(ModuleNames.DASHBOARDS) | |||
getModuleActiveStatus(ModuleNames.DASHBOARDS) || | |||
getModuleActiveStatus(ModuleNames.DATASETS_BASE) |
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.
minor: getModuleActiveStatus(ModuleNames.DATASETS_BASE)
will always return false in datasets_base
not in config.json
does not affect logic though
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 need to review that, because maybe not now but I am counting on being able to define all datasets enabled like this.... I'll have a second look because by the way ModuleNames and dependencies are defined it should work
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.
looks good! Approving
Feature or Bugfix
Detail
As explained in the design for #1123 we are trying to implement a generic
datasets_base
module that can be used by any type of datasets in a generic way.In this PR we:
If we want to keep everything clean we could rename all "datasets" as "s3_datasets" or equivalent in the S3_Datasets module. Because it is a cosmetic change that would pollute the PR a lot I have decided not to include it.
The Data Shared With You table in Environments>Datasets tab needs a remake. It contains references to each type of item and it is very coupled with s3-dataset-shares. For the moment I just made it work for the changes of s3-datasets, but when completing the work in #1283 we should fix this. Maybe in favor of DataGrid
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.