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

Address Microsoft feedback #35

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

Address Microsoft feedback #35

wants to merge 35 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 8, 2022

Description

There was a PBI connector issue found when a fix to an ssl issue was being resolved. The solution to the connector issue was to create a Certificate Validation option for users specified in PR 449. Microsoft came back with the feedback saying "It appears the update is adding a new required parameter the data source function signatures, which also makes it part of the credential path. This would be a breaking change for existing queries and existing credentials.".

The solution in this PR is to make Certificate Validation optional with a default of true when no option is selected. This adds a parameter to the pbix files made with the old connector and allows to ability to successfully load old pbix files with the proposed changes to the connector.

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Guian Gumpac and others added 30 commits November 17, 2021 13:48
Added Tableau Connector to OpenSearch SQL
Signed-off-by: Guian Gumpac <[email protected]>
Fix for ADDDATE and SUBDATE with resulting 00:00:00 being unexpectedly null
Signed-off-by: Yury Fridlyand <[email protected]>
Dialect: Added CAST to convert to int and string as of AOS-202
Signed-off-by: Guian Gumpac <[email protected]>
Dialect: fix return type for MID function as part of AOS-202
Dialect: fixes math.hexbin function as of AOS-195
Signed-off-by: Guian Gumpac <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Pull most recent changes from the upstream project
Guian Gumpac and others added 5 commits February 25, 2022 11:16
* Added option for Hostname Verification for PBI connector

Signed-off-by: Guian Gumpac <[email protected]>

* Changed Beta to false and updated screenshots on the readme files

Signed-off-by: Guian Gumpac <[email protected]>

* Bump connector version and add CHANGELOG

Signed-off-by: Guian Gumpac <[email protected]>

* Improved changelog message for 1.0.1 PBI connector

Signed-off-by: Guian Gumpac <[email protected]>

* Updated PBIDS Handler for the connector

Signed-off-by: Yury Fridlyand <[email protected]>

Co-authored-by: Yury Fridlyand <[email protected]>
@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #35 (10fd46c) into main (278c05f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main      #35   +/-   ##
=========================================
  Coverage     99.92%   99.92%           
  Complexity     2698     2698           
=========================================
  Files           257      257           
  Lines          6491     6491           
  Branches        402      402           
=========================================
  Hits           6486     6486           
  Misses            5        5           
Flag Coverage Δ
sql-engine 99.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef5597e...10fd46c. Read the comment docs.

@ghost ghost requested a review from stevelorddremio March 8, 2022 18:10
@@ -82,7 +82,11 @@ OpenSearchProjectImpl = (Server as text, Port as number, UseSSL as logical, Host
ConnectionString = [
Driver = "OpenSearch SQL ODBC Driver",
Host = FinalServerString,
HostnameVerification = if HostnameVerification then 1 else 0
HostnameVerification =
if HostnameVerification = null or HostnameVerification then
Copy link

Choose a reason for hiding this comment

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

I think you can use the ? operator to check for nullity + get the value.

Copy link

Choose a reason for hiding this comment

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

Does there need to be a change for PBIDS files to specify this option?

@@ -28,7 +28,7 @@ OpenSearchProjectType = type function (
Documentation.FieldDescription = "Use SSL",
Documentation.AllowedValues = { true, false }
]),
HostnameVerification as (type logical meta [
optional HostnameVerification as (type logical meta [

Choose a reason for hiding this comment

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

Have you tested opening PBIDS files generated with the latest connector to see if they work if the optional parameter is not given a value?
There could also be issues with TestConnection as it is expecting HostnameValidation to be set. You need to test with Gateway to confirm.

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.

5 participants