Skip to content

Bug fixes after 2-7-0-rc #1785

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Bug fixes after 2-7-0-rc #1785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
894a725
Fixes for bugs
TejasRGitHub Mar 12, 2025
1682a0e
Adding maintenance window fixes
TejasRGitHub Mar 12, 2025
c0d19b8
bumping version of axios
TejasRGitHub Mar 12, 2025
a766e87
Adding package-lock.json and yarn.lock
TejasRGitHub Mar 12, 2025
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
10 changes: 6 additions & 4 deletions backend/dataall/base/aws/parameter_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import logging
from typing import List

from botocore.exceptions import ClientError

Expand Down Expand Up @@ -39,15 +40,16 @@ def get_parameter_value(AwsAccountId=None, region=None, parameter_path=None):

@staticmethod
def get_parameters_by_path(AwsAccountId=None, region=None, parameter_path=None):
parameter_value_list: List[str] = []
if not parameter_path:
raise Exception('Parameter name is None')
try:
parameter_values = ParameterStoreManager.client(AwsAccountId, region).get_parameters_by_path(
Path=parameter_path
)['Parameters']
paginator = ParameterStoreManager.client(AwsAccountId, region).get_paginator('get_parameters_by_path')
for page in paginator.paginate(Path=parameter_path):
parameter_value_list.extend(page['Parameters'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be the following instead to avoid KeyError on 'Parameters' if it does not exist, unless you are really wanting that exception if it doesn't exist:

parameter_value_list.extend(page.get('Parameters', []))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Umm. Taking a look at the API response ,https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ssm/client/get_parameters_by_path.html, Parameters is always returned ( return empty list [] if nothing is found). Since this contract is declared, I am expecting Parameters to always be present. Also, if Parameters is not present, then I would want this function to raise an exception which bubbles up to the user enabling maintenance window.

except ClientError as e:
raise Exception(e)
return parameter_values
return parameter_value_list

@staticmethod
def update_parameter(AwsAccountId, region, parameter_name, parameter_value):
Expand Down
4 changes: 2 additions & 2 deletions backend/dataall/base/utils/naming_convention.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ class NamingConventionPattern(Enum):
IAM = {'regex': '[^a-zA-Z0-9-_]', 'separator': '-', 'max_length': 63} # Role names up to 64 chars
IAM_POLICY = {'regex': '[^a-zA-Z0-9-_]', 'separator': '-', 'max_length': 128} # Policy names up to 128 chars
GLUE = {
'regex': '[^a-zA-Z0-9_]',
'regex': '[^a-zA-Z0-9_-]',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding "-" into the regex

'separator': '_',
Copy link
Collaborator

Choose a reason for hiding this comment

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

But we are specifying separator as '_'.
What will happen if we have dashes in the name? It wont detect the separator as '-'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The separator field is used to create a data.all generated name ( like dataall_glue_db, etc ) when we create a dataset and data.all creates the S3 bucket, gluedb, etc.

The regex field is used when making a name that is compliant ( with that regex ). In such cases, when data.all creates a name, and uses "_" as separator then that would be a valid name since it is part of the character set defined by regex

'max_length': 240,
'valid_external_regex': '^[a-zA-Z0-9_]+$',
'valid_external_regex': '^[a-zA-Z0-9_-]+$',
} # Limit 255 - 15 extra chars buffer
GLUE_ETL = {'regex': '[^a-zA-Z0-9-]', 'separator': '-', 'max_length': 52}
NOTEBOOK = {'regex': '[^a-zA-Z0-9-]', 'separator': '-', 'max_length': 63}
Expand Down
2 changes: 1 addition & 1 deletion backend/dataall/modules/s3_datasets/cdk/assets/glueprofilingjob/glue_script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def run_table_profiling(
logger.debug('Profiling table for %s %s ', database, table)
logger.debug('using %s', database)
spark.sql('use `{}`'.format(database))
df = spark.sql('select * from {}'.format(table))
df = spark.sql('select * from `{}`'.format(table))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Minor correction

Copy link
Collaborator

Choose a reason for hiding this comment

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

could also just be:

df = spark.sql(f"SELECT * FROM {table}")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I could do this as well. I just wanted add the `` -

df = spark.sql(f"SELECT * FROM `{table}`")

total = df.count()
logger.debug('Retrieved count for %s %s', table, total)

Expand Down
2 changes: 0 additions & 2 deletions deploy/stacks/pipeline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,6 @@ def validate_deployment_params(self, source, repo_connection_arn, git_branch, re
or 'client_id' not in custom_auth_configs
or 'response_types' not in custom_auth_configs
or 'scopes' not in custom_auth_configs
or 'jwks_url' not in custom_auth_configs
or 'claims_mapping' not in custom_auth_configs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing jwks_url from pipeline validation as it is no longer used

or 'user_id' not in custom_auth_configs['claims_mapping']
or 'email' not in custom_auth_configs['claims_mapping']
Expand All @@ -442,7 +441,6 @@ def validate_deployment_params(self, source, repo_connection_arn, git_branch, re
or not isinstance(custom_auth_configs['client_id'], str)
or not isinstance(custom_auth_configs['response_types'], str)
or not isinstance(custom_auth_configs['scopes'], str)
or not isinstance(custom_auth_configs['jwks_url'], str)
or not isinstance(custom_auth_configs['claims_mapping']['user_id'], str)
or not isinstance(custom_auth_configs['claims_mapping']['email'], str)
):
Expand Down
8 changes: 4 additions & 4 deletions frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"apollo-boost": "^0.4.9",
"aws-amplify": "^5.3.14",
"braces": "3.0.3",
"axios": "^1.7.4",
"axios": "^1.8.2",
"classnames": "^2.3.1",
"date-fns": "^2.28.0",
"dayjs": "^1.11.0",
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/authentication/components/MaintenanceGuard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@
}
};

useEffect(async () => {
useEffect(() => {
// Check if the maintenance window is enabled and has NO-ACCESS Status
// If yes then display a blank screen with a message that data.all is in maintenance mode ( Check use of isNoAccessMaintenance state )
if (isModuleEnabled(ModuleNames.MAINTENANCE) === true) {
if (client) {
if (client && groups) {
checkMaintenanceMode().catch((e) => dispatch({ type: SET_ERROR, e }));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding groups check, so that the UI is correctly displayed. Without this, when data.all is in No-Access maintenance mode, No-access maintenance window is shown for a fraction of second and then goes away and proper UI is rendered

}
}
}, [client, groups]);

Check warning on line 45 in frontend/src/authentication/components/MaintenanceGuard.js

View workflow job for this annotation

GitHub Actions / es-lint (20.x) 

React Hook useEffect has missing dependencies: 'checkMaintenanceMode' and 'dispatch'. Either include them or remove the dependency array

if (
isNoAccessMaintenance == null &&
Expand Down
6 changes: 5 additions & 1 deletion frontend/src/modules/Administration/views/AdministrationView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import { useState } from 'react';
import { Helmet } from 'react-helmet-async';
import { Link as RouterLink } from 'react-router-dom';
import { ChevronRightIcon, useSettings } from 'design';
import { ChevronRightIcon, LoadingScreen, useSettings } from 'design';
import { AdministrationTeams, DashboardViewer } from '../components';
import { MaintenanceViewer } from '../../Maintenance/components/MaintenanceViewer';
import { isModuleEnabled, ModuleNames, isTenantUser } from 'utils';
Expand All @@ -42,6 +42,10 @@
setCurrentTab(value);
};

if (!groups) {
return <LoadingScreen />;
}

return !isTenantUser(groups) ? (
<Box
sx={{
Expand Down Expand Up @@ -135,4 +139,4 @@
);
};

export default AdministrationView;

Check warning on line 142 in frontend/src/modules/Administration/views/AdministrationView.js

View workflow job for this annotation

GitHub Actions / es-lint (20.x) 

Prefer named exports
16 changes: 10 additions & 6 deletions frontend/src/modules/Maintenance/components/MaintenanceViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ export const ReIndexConfirmationPopUp = (props) => {

export const MaintenanceViewer = () => {
const client = useClient();
const [refreshing, setRefreshing] = useState(false);
const [refreshing, setRefreshing] = useState(true);
const refreshingReIndex = false;
const [updatingReIndex, setUpdatingReIndex] = useState(false);
const [updating, setUpdating] = useState(false);
Expand Down Expand Up @@ -443,7 +443,6 @@ export const MaintenanceViewer = () => {
};

const initializeMaintenanceView = useCallback(async () => {
setRefreshing(true);
const response = await client.query(getMaintenanceStatus());
if (!response.errors && response.data.getMaintenanceWindowStatus !== null) {
const maintenanceStatusData = response.data.getMaintenanceWindowStatus;
Expand Down Expand Up @@ -471,13 +470,10 @@ export const MaintenanceViewer = () => {
dispatch({ type: SET_ERROR, error });
}
setRefreshing(false);
}, [client]);
}, [client, maintenanceModes]);

useEffect(() => {
if (client) {
initializeMaintenanceView().catch((e) =>
dispatch({ type: SET_ERROR, e })
);
fetchMaintenanceModes().catch((e) =>
dispatch({ type: SET_ERROR, error: e.message })
);
Expand All @@ -491,6 +487,14 @@ export const MaintenanceViewer = () => {
}
}, [client]);

useEffect(() => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #1778 (comment) for more details

if (maintenanceModes.length > 0) {
initializeMaintenanceView().catch((e) =>
dispatch({ type: SET_ERROR, e })
);
}
}, [maintenanceModes]);

return (
<Box>
{refreshingReIndex ? (
Expand Down
8 changes: 4 additions & 4 deletions frontend/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6104,10 +6104,10 @@ axe-core@=4.7.0:
resolved "https://registry.npmjs.org/axe-core/-/axe-core-4.7.0.tgz"
integrity sha512-M0JtH+hlOL5pLQwHOLNYZaXuhqmvS8oExsqB1SBYgA4Dk7u/xx+YdGHXaK5pyUfed5mYXdlYiphWq3G8cRi5JQ==

axios@^1.6.5, axios@^1.7.4:
version "1.7.4"
resolved "https://registry.npmjs.org/axios/-/axios-1.7.4.tgz"
integrity sha512-DukmaFRnY6AzAALSH4J2M3k6PkaC+MfaAGdEERRWcC9q3/TWQwLpHR8ZRLKTdQ3aBDL64EdluRDjJqKw+BPZEw==
axios@^1.6.5, axios@^1.8.2:
version "1.8.3"
resolved "https://registry.npmjs.org/axios/-/axios-1.8.3.tgz"
integrity sha512-iP4DebzoNlP/YN2dpwCgb8zoCmhtkajzS48JvwmkSkXvPI3DHc7m+XYL5tGnSlJtR6nImXZmdCuN5aP8dh1d8A==
dependencies:
follow-redirects "^1.15.6"
form-data "^4.0.0"
Expand Down
Loading