Skip to content

[Feature][task-plugins-12820] azure azure-sql datasource plugins#2

Closed
Tianqi-Dotes wants to merge 11 commits intodevfrom
feature/azure-sql
Closed

[Feature][task-plugins-12820] azure azure-sql datasource plugins#2
Tianqi-Dotes wants to merge 11 commits intodevfrom
feature/azure-sql

Conversation

@Tianqi-Dotes
Copy link
Copy Markdown
Owner

Purpose of the pull request

apache#12820
supports 5 auth mode to azure sql

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@github-actions github-actions bot added the UI label Dec 16, 2022
return jdbcUrl + ";authentication=" + param.getMode().getDescp();
case AD_MSI:
if (StringUtils.isEmpty(param.getMSIClientId())) {
return jdbcUrl + ";authentication=" + param.getMode().getDescp();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's better to use String.format

return jdbcUrl + ";authentication=" + param.getMode().getDescp();
} else {
// write MSIClientId inside jdbc URL so no need MSIClientId in the AzureSQLConnectionParam
return jdbcUrl + ";authentication=" + param.getMode().getDescp()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same here.


private void checkTrustServerCertificate(BaseDataSourceParamDTO paramDTO){
Map<String, String> other = Optional.ofNullable(paramDTO.getOther()).orElseGet(HashMap::new);
other.put("trustServerCertificate","true");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please add some comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants