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

[ui-storagebrowser] moves the HDFS config to /fileSystem API #3987

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

ramprasadagarwal
Copy link
Collaborator

What changes were proposed in this pull request?

  • The config of file system has been moved from /list_dir API to /filesystem API.

How was this patch tested?

  • Manually tested

Please review Hue Contributing Guide before opening a pull request.

@ramprasadagarwal ramprasadagarwal self-assigned this Feb 6, 2025
@ramprasadagarwal ramprasadagarwal marked this pull request as ready for review February 6, 2025 11:49
@ramprasadagarwal ramprasadagarwal enabled auto-merge (squash) February 12, 2025 18:02
const urlPathname = window.location.pathname;
const urlSearchParams = new URLSearchParams(window.location.search);
const urlFilePath = decodeURIComponent(urlSearchParams.get('path') ?? '');
const { fileSystem: urlFileSystem } = getFileSystemAndPath(urlFilePath);
const initialFilePath = urlFileSystem === fileSystem ? urlFilePath : homeDir;
const initialFilePath =
urlFileSystem === fileSystem.file_system ? urlFilePath : fileSystem.user_home_directory;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a naming problem here: fileSystem.file_system

I would also like to avoid snake_case in the UI. We use cameCase in all naming so we should try and stick to that. Sometimes we get properties from the backend that uses snake_case but it is a simple tasks to rename them before passing them a along to the rest of the UI code.

groups?: ListDirectory['groups'];
isFsSuperUser?: ListDirectory['is_fs_superuser'];
isTrashEnabled?: boolean;
config: FileSystem['config'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we expect that the input of StorageBrowserRowActionsProps will stay aligned with whatever we define FileSystem['config'] as? I'm asking because if we expect that FileSystem['config'] might evolve into a superset of what we actually need here then this will not add much value.

@@ -124,7 +113,7 @@ const StorageBrowserActions = ({
};

const actionItems: MenuItemType[] = useMemo(() => {
const enabledActions = getEnabledActions(t, selectedFiles, isFsSuperUser);
const enabledActions = getEnabledActions(t, selectedFiles, config?.is_hdfs_superuser);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a case where we are going to show action modals like the DeletionModal without ever getting the config?

If the config is just arriving during a later rendering cycle then should we even show those related modals before we have the config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This won't be a case, we get this config from the /fileSystem API which is the first and foremost call made in the storage-browser. After getting the response we make subsequent calls and show the action buttons on list directory page.

label: string;
key: number;
icon: React.ReactNode;
user_home_dir: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: camelCase

@ramprasadagarwal ramprasadagarwal merged commit 42307bf into master Feb 13, 2025
6 checks passed
@ramprasadagarwal ramprasadagarwal deleted the fix/storage-browser-25 branch February 13, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants