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

Fix/ldap password #396

Merged
merged 5 commits into from
Oct 11, 2022
Merged

Fix/ldap password #396

merged 5 commits into from
Oct 11, 2022

Conversation

VShkaberda
Copy link
Contributor

resolves #310
Created instead of #311

Description

Add password to build_ssl_transport and hive.connect. Allows to use LDAP authentication. Password is optional, so that it remains None for other auth if not specified in profiles.yml.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-spark next" section.

@cla-bot cla-bot bot added the cla:yes label Jul 19, 2022
@jtcohen6
Copy link
Contributor

Copying from #311 (comment):

The changes look straightforward, although ldap authentication isn't something we have automated testing for today.

Have you been able to confirm that this works for you, with your own cluster, when running from this branch locally?

@VShkaberda
Copy link
Contributor Author

@jtcohen6 Yes, I can confirm it. We have Apache Kyuubi with LDAP authentication and these are the changes we made to be able to connect via thrift. We don't use ssl though, so I can't confirm it actually works with it, but I don't see significant differences making it non-working with ssl.

@jtcohen6 jtcohen6 added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Aug 1, 2022
Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM! @VShkaberda thanks for contributing it!

@nousot-cloud-guy nousot-cloud-guy mentioned this pull request Aug 4, 2022
2 tasks
@VShkaberda
Copy link
Contributor Author

Hello, do you plan to merge this changes until the next release? There is a feeling that this PR has been forgotten. I saw an old comment in the issue thread

We should ultimately document what this method does

I'm not sure if it could be a blocker.

@nousot-cloud-guy
Copy link

I'm also interested in getting this PR merged. Is there anything blocking it at the moment?

@leahwicz leahwicz closed this Sep 21, 2022
@leahwicz leahwicz reopened this Sep 21, 2022
@leahwicz
Copy link
Contributor

Retriggering CI

@leahwicz
Copy link
Contributor

@VShkaberda sorry for the delay in merging here! Would you be able to generate a changelog entry (instructions here) and then I can merge this in for the 1.3 release

@VShkaberda
Copy link
Contributor Author

I tried to use dockerized changie, but had no success generating CHANGELOG. I'll try again and update this thread thereafter. PS. body in actions has broken link to dbt-spark contributing guide

@leahwicz
Copy link
Contributor

@VShkaberda if you can't get it to work, just let me know and I can copy your commits to a branch and generate the changelog so we can get this in

@VShkaberda
Copy link
Contributor Author

@leahwicz I end up installing changie and still can't figure out how to make it work. Trying changie new (as it is said in CONTRIBUTING.md) just gives me Error: open .changie.yml: No such file or directory. So I executed changie init and .chages/ and .changie.yaml were created and CHANGELOG.md was cleared. I'm not sure if this is how it is supposed to be. If it is I think it's a good idea to make it more clear in the CONTRIBUTING.md.

@leahwicz
Copy link
Contributor

@VShkaberda I think I see what happened. The fork you were working on did not have the latest changie updates to it so that's why it was not working correctly. I fixed things up here so 🤞 that CI will pass and we can merge this in

@leahwicz leahwicz mentioned this pull request Sep 26, 2022
6 tasks
@McKnight-42
Copy link
Contributor

@ChenyuLInx seems this one is still failing for a python_model issue was curious if you had any ideas? or if this is a nuisance problem?

@ChenyuLInx
Copy link
Contributor

@McKnight-42 sorry for seeing this late, I think we can merge it, I think the tests might be running with this branch, but we fixed that python tests on the main branch already

@ChenyuLInx ChenyuLInx merged commit 37dcfe3 into dbt-labs:main Oct 11, 2022
@nousot-cloud-guy
Copy link

@ChenyuLInx did this make it into the 1.3 release a few days ago?

@leahwicz
Copy link
Contributor

@nousot-cloud-guy sorry but this did not make it into the 1.3 release 😞

@nousot-cloud-guy
Copy link

@leahwicz I'm guessing it'll be in 1.3 then. Any idea when that might be?

@nousot-cloud-guy
Copy link

Sorry I meant 1.4.

@leahwicz
Copy link
Contributor

@nousot-cloud-guy yes- it will be in the 1.4 release which will be at the end of January

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-444] LDAP connection is broken
6 participants