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

feat: add datasource config in import module #509

Merged
merged 17 commits into from
Mar 31, 2023

Conversation

hetao92
Copy link
Contributor

@hetao92 hetao92 commented Mar 16, 2023

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

return _delete(`/api/datasources/${id}`)(undefined, config);
},
batchDeleteDatasource: (payload, config?) => {
return _delete(`/api/datasources`)(undefined, { data: payload });
Copy link
Contributor

Choose a reason for hiding this comment

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

recommand: undefined => {}?

endpoint: string;
accessKey: string;
accessSecret: string;
bucket?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is required now?

accessSecret: string;
bucket?: string;
token?: string;
key: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the token and key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are the parameters of the importer, key is the file path

Comment on lines +74 to +79
keyFile?: string;
keyData?: string;
passPhrase?: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what are they used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep the configuration definition the same as the nebula importer, but it is not currently used in the studio

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are not used in our app, you could not pre-define them now, maybe we will never use these fields, it's little redundant.

<FormItem name="platform" rules={[{ required: true, message: intl.get('formRules.platformRequired') } ]}>
<Select placeholder={intl.get('import.selectPlatform')}>
<Select.Option value="aws">AWS S3</Select.Option>
<Select.Option value="aliyun">OSS</Select.Option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Aliyun OSS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tecent COS

Copy link
Contributor

Choose a reason for hiding this comment

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

S3 is a standard, also should consider the local s3 service

Copy link
Contributor

Choose a reason for hiding this comment

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

recommend to use one component per file, easy to understand

Comment on lines 64 to 70
return nil, ecode.WithBadRequest(err, "json stringify error")
return nil, ecode.WithErrorMessage(ecode.ErrBadRequest, err, "json stringify error")
Copy link
Contributor

Choose a reason for hiding this comment

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

WithBadRequest is more short and proper?

bucket = pathParts[1]
}
} else {
// Format: https://<bucket-name>.s3.<region>.amazonaws.com
Copy link
Contributor

Choose a reason for hiding this comment

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

can it also work for aliyun oss?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I split the oss logic out from here

}
defer conn.Close()

// create an SFTP client session
client, err := sftp.NewClient(conn)
if err != nil {
return ecode.WithBadRequest(err, "failed to create SFTP session")
return ecode.WithErrorMessage(ecode.ErrBadRequest, err, "failed to create SFTP session")
Copy link
Contributor

Choose a reason for hiding this comment

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

ecode.WithBadRquest is the short-mode of ecode.WithErrorMessage(ecode.ErrBadRequest....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
image
its not same

Copy link
Contributor

Choose a reason for hiding this comment

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

@veezhang why are they different? I previously think that the WithBadRequest is the shortcut of ht WithErrorMessage(ecode.ErrBadRequest...

nianiaJR
nianiaJR previously approved these changes Mar 26, 2023
Copy link
Contributor

@nianiaJR nianiaJR left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 1 to 11
export enum IDatasourceType {
's3' = 's3',
'sftp' = 'sftp',
'local' = 'local'
}
export enum IS3Platform {
'aws' = 'aws',
'oss' = 'oss',
'customize' = 'customize',
'tecent' = 'cos'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

export enum DatasourceType {
  S3 = 's3',
  SFTP = 'sftp',
  Local = 'local'
}

export enum S3Platform {
  AWS = 'aws',
  OSS = 'oss',
  Tecent = 'cos',
  Customize = 'customize',
}

Comment on lines 13 to 35
export interface IDatasourceAdd {
name: string;
type: IDatasourceType;
platform?: string;
s3Config?: {
accessKey: string;
accessSecret?: string;
endpoint: string;
bucket: string;
region?: string;
};
sftpConfig?: {
host: string;
port: number;
username: string;
password: string;
};
}
export interface IDatasourceUpdate extends IDatasourceAdd {
id: number;
}

export interface IDatasourceItem {
id?: number;
name: string;
type: IDatasourceType;
platform?: string;
createTime?: string;
s3Config?: {
accessKey: string;
accessSecret?: string;
endpoint: string;
bucket: string;
region?: string;
};
sftpConfig?: {
host: string;
port: number;
username: string;
password: string;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

repetitive content between IDatasourceAdd and IDatasourceItem

Comment on lines 36 to 69
const submit = async (values: IDatasourceItem) => {
const _type = values.type || type;
setLoading(true);
if(mode === 'create') {
const flag = await addDataSource({
type: _type,
name: '',
...values
});
setLoading(false);
flag && (message.success(intl.get('schema.createSuccess')), onConfirm());
} else {
let _config;
switch (_type) {
case IDatasourceType.s3:
_config = values.s3Config;
_config.accessSecret === tempPwd && delete _config.accessSecret;
break;
case IDatasourceType.sftp:
_config = values.sftpConfig;
_config.password === tempPwd && delete _config.password;
break;
default:
break;
}
const flag = await updateDataSource({
id: data.id,
type: _type,
name: '',
...values
});
setLoading(false);
flag && (message.success(intl.get('common.updateSuccess')), onConfirm());
}
};
Copy link
Contributor

@huaxiabuluo huaxiabuluo Mar 27, 2023

Choose a reason for hiding this comment

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

const submit = async (values: IDatasourceItem) => {
  setLoading(true);
  const payload = { type: values.type || type, name: '', ...values };

  if (mode === 'create') {
    const flag = await addDataSource(payload);
    setLoading(false);
    flag && (message.success(intl.get('common.updateSuccess')), onConfirm());
    return;
  }

  switch (payload.type) {
    case IDatasourceType.s3: {
      const c = payload.s3Config;
      payload.s3Config = c?.accessSecret === tempPwd ? { ...c, accessSecret: undefined } : c;
      break;
    }
    case IDatasourceType.sftp: {
      const c = payload.sftpConfig;
      payload.sftpConfig = c?.password === tempPwd ? { ...c, password: undefined } : c;
      break;
    }
    default:
      break;
  }
  const flag = await updateDataSource({ id: data.id, ...payload });
  // const flag = await (() => {
  //   if (mode === 'create') {
  //     return addDataSource(payload);
  //   }

  //   switch (payload.type) {
  //     case IDatasourceType.s3: {
  //       const c = payload.s3Config;
  //       payload.s3Config = c?.accessSecret === tempPwd ? { ...c, accessSecret: undefined } : c;
  //       break;
  //     }
  //     case IDatasourceType.sftp: {
  //       const c = payload.sftpConfig;
  //       payload.sftpConfig = c?.password === tempPwd ? { ...c, password: undefined } : c;
  //       break;
  //     }
  //     default:
  //       break;
  //   }
  //   return updateDataSource({ id: data.id, ...payload });
  // })();

  setLoading(false);
  flag && (message.success(intl.get('common.updateSuccess')), onConfirm());
};

Comment on lines 18 to 57
const cloudKeys = [
{
title: 'ipAddress',
key: ['s3Config', 'endpoint']
},
{
title: 'bucketName',
key: ['s3Config', 'bucket']
},
{
title: 'accessKeyId',
key: ['s3Config', 'accessKey']
},
{
title: 'region',
key: ['s3Config', 'region']
},
{
title: 'createTime',
key: 'createTime',
render: data => data && dayjs(data).format('YYYY-MM-DD HH:mm:ss')
},
];
const sftpKeys = [{
title: 'ipAddress',
key: ['sftpConfig', 'host'],
render: (_, row) => `${row.sftpConfig.host}:${row.sftpConfig.port}`
},
{
title: 'account',
key: ['sftpConfig', 'username']
},
{
title: 'createTime',
key: 'createTime',
render: data => data && dayjs(data).format('YYYY-MM-DD HH:mm:ss')
}
];
Copy link
Contributor

Choose a reason for hiding this comment

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

const cloudColumns: TableColumnType<IDatasourceItem>[] = [
  {
    title: <Translation>{(i) => i.get('import.ipAddress')}</Translation>,
    dataIndex: ['s3Config', 'endpoint'],
  },
  {
    title: <Translation>{(i) => i.get('import.bucketName')}</Translation>,
    dataIndex: ['s3Config', 'bucket'],
  },
  {
    title: <Translation>{(i) => i.get('import.accessKeyId')}</Translation>,
    dataIndex: ['s3Config', 'accessKey'],
  },
  {
    title: <Translation>{(i) => i.get('import.region')}</Translation>,
    dataIndex: ['s3Config', 'region'],
  },
  {
    title: <Translation>{(i) => i.get('import.createTime')}</Translation>,
    dataIndex: 'createTime',
    render: (data) => data && dayjs(data).format('YYYY-MM-DD HH:mm:ss'),
  },
];

const sftpColumns: TableColumnType<IDatasourceItem>[] = [
  {
    title: <Translation>{(i) => i.get('import.ipAddress')}</Translation>,
    dataIndex: ['sftpConfig', 'host'],
    render: (_, row) => `${row.sftpConfig.host}:${row.sftpConfig.port}`,
  },
  {
    title: <Translation>{(i) => i.get('import.account')}</Translation>,
    dataIndex: ['sftpConfig', 'username'],
  },
  {
    title: <Translation>{(i) => i.get('import.createTime')}</Translation>,
    dataIndex: 'createTime',
    render: (data) => data && dayjs(data).format('YYYY-MM-DD HH:mm:ss'),
  },
];

Comment on lines 84 to 106
const columns = columnKeys[type].map(item => ({
title: intl.get(`import.${item.title}`),
dataIndex: item.key,
render: item.render
})).concat(({
title: intl.get('common.operation'),
key: 'operation',
render: (_, item: IDatasourceItem) => (<div className={styles.operation}>
<Button className="primaryBtn" onClick={() => editItem(item)}>
<Icon type="icon-studio-btn-detail" />
</Button>
<Popconfirm
onConfirm={() => deleteItem(item.id)}
title={intl.get('common.ask')}
okText={intl.get('common.confirm')}
cancelText={intl.get('common.cancel')}
>
<Button className="warningBtn">
<Icon type="icon-studio-btn-delete" />
</Button>
</Popconfirm>
</div>)
} as any));
Copy link
Contributor

Choose a reason for hiding this comment

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

const tableColumns: TableColumnType<IDatasourceItem>[] = useMemo(() => {
  const columns = type === IDatasourceType.s3 ? cloudColumns : sftpColumns;
  return columns.concat({
    title: intl.get('common.operation'),
    key: 'operation',
    render: (_, item) => (
      <div className={styles.operation}>
        <Button className="primaryBtn" onClick={() => editItem(item)}>
          <Icon type="icon-studio-btn-detail" />
        </Button>
        <Popconfirm
          onConfirm={() => deleteItem(item.id)}
          title={intl.get('common.ask')}
          okText={intl.get('common.confirm')}
          cancelText={intl.get('common.cancel')}
        >
          <Button className="warningBtn">
            <Icon type="icon-studio-btn-delete" />
          </Button>
        </Popconfirm>
      </div>
    ),
  });
}, [type, intl]);

Comment on lines 73 to 75
const _path = path.slice(0, -1).split('/');
_path.pop();
const newPath = _path.join('/').length ? _path.join('/') + '/' : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

/**
 * - `/a/b/c/?` => `/a/b/`
 * - `a/b/?` => `a`
 * - `a/?` => ``
 */
const parentPath = path.replace(/(\/|^)[^/]+\/?$/, '$1');

Comment on lines 100 to 123
const handleConfirm = useCallback(async () => {
if(activeItem && activeId === IDatasourceType.local) {
// select local file
onConfirm(activeItem, state);
} else {
setState({ loading: true });
const _path = `${path === '/' ? '' : path}${activeItem.name}`;
const data = await previewFile({ id: activeId, path: _path });
const item = {
name: activeItem.name,
withHeader: false,
delimiter: ',',
sample: data.contents.join('\r\n'),
path: _path,
datasourceId: activeId === IDatasourceType.local ? null : activeId,
} as any;
setState({ loading: false });
onConfirm(item, state);
}
}, [activeItem, activeId, path]);
Copy link
Contributor

Choose a reason for hiding this comment

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

early return, unnecessary else

Copy link
Contributor

@nianiaJR nianiaJR left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants