-
Notifications
You must be signed in to change notification settings - Fork 81
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
[Gh-1032] Feature flags for topics and confidentiality and custom confidentiality list #1049
[Gh-1032] Feature flags for topics and confidentiality and custom confidentiality list #1049
Conversation
Thanks for opening @TejasRGitHub - I did an initial review of the PR and left some comments to address - I'll test this out in a deployment tomorrow |
Updated PR with the changes requested. Testing
|
Testing on AWS: With Dropdowns enabled (Topics and confidentiality) & Custom Confidentiality Mapping As:
With Dropdowns Disabled (Topics and Confidentiality):
With Dropdowns Enabled (No Custom Mapping):
Notes:
|
I finished testing and called out some notes in the above comment - I think the only point that needs fixing before PR approval is the dataset_stack creation Error due to missing tag value on dataset.confidentiality Otherwise, I think the behaviors of how we are treating pre-existing datasets when switching to custom confidentiality or disabling altogether is fine (it opts to the more restritive option rather than One final suggestion is to rename |
Hi @noah-paige , Thanks for catching that. I must have missed that during testing. For the notes, about behaviours ( when a drop down is not present / changing the behaviour from custom to standard) I agree that the behaviour is restrictive and I don't have any issue with it as such. Regarding your suggestion, I understand why you made this suggestion, but the configs i.e. Apart from that, your notes made me realize that the catalog search would be rendered useless if the custom mapping names have a space. For that I suggest we remove the spaced part in the name and make it whole while indexing it at I will fix the error pointed out by you and push the updated code. Before that please let me know if you agree with the above as well |
The above sounds good to me - please go ahead making those final changes |
@@ -34,7 +36,7 @@ def upsert(cls, session, dataset_uri: str): | |||
'source': dataset.S3BucketName, | |||
'resourceKind': 'dataset', | |||
'description': dataset.description, | |||
'classification': dataset.confidentiality, | |||
'classification': re.sub('[^A-Za-z0-9]+', '', dataset.confidentiality), |
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.
Adding this to update the confidentiality to remove any special characters/spaces
Hi @noah-paige , Updated the code. TestingDeployed data.all with Confidentiality is disabled and Topics is enabled.Able to create Dataset ✅ Deployed data.all with Confidentiality is Enabled and Topics is EnabledAble to edit the same dataset ✅ Deployed data.all with Confidentiality is Enabled and Topics is Enabled and using custom mapping which contain spaces and '-' and some special charactersAble to create Dataset . And able to see the custom confidentiality string ✅ |
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.
thanks for making the iterative changes @TejasRGitHub - this PR looks good to me!
Hi @noah-paige , Thanks for approving. |
Feature or Bugfix
Detail
This features adds feature flags to enable / disable topics and confidentiality fields while creating a dataset. Moreover, it adds ways to provide custom confidentiality fields to show on the UI.
Please note - For the custom confidentiality fields you will have to provide the mapping to the existing confidentiality levels ( i.e. Unclassified, Secret, Official ) in data.all . Please refer to this section about setting data.all for more details - https://data-dot-all.github.io/dataall/deploy-aws/
Testing
Things to do
How to use this feature.
The config.json is already updated with the new configs , if you want to use the custom mapping , please add the following section in the config.json under the
modules \ datasets \ features
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)? No
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.