-
Notifications
You must be signed in to change notification settings - Fork 7
Add LDAP tests #4
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
base: master
Are you sure you want to change the base?
Conversation
this is needed, because - since it became configurable - JDBC driver defaults to a jar in one of the target dirs.
cawallin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, though @ArturGajowy should probably review it too
|
I'll squash the three commits together before merging... |
|
I've seen the checks passed for sprint-43, but there is no trace of ldap tests run in the logs - as if the ldap tests weren't merged to the sprint branch (https://travis-ci.org/Teradata/presto-checks/jobs/174016150). We need to check they pass - I'll trigger a build for sprint-42. Edit: WHOAH sorry my bad I read the logs wrong. There are indeed logs of LDAP tests passing :) Edit: build started here https://travis-ci.org/Teradata/presto-checks/builds/174054556 |
|
About the JDBC driver - please rebase this branch on |
|
After we see the tests run and pass and after this branch is rebased, we'll rebase branches cdh_latest and hdp_latest on top of this, so that we can have the ldap tests run on both distros (also, that's exactly what we're testing the sprint branch against). This will be the equivalent of merging this pull request (after that, the builds visible on our dashboard will include the ldap tests). We won't be merging the |
|
So, the tests passed for both sprint-42 and 43. Please rebase on feature/overridable_jdbc_driver and it LGTM :) After rebasing, please rebase the EDIT: if you push-force, please do it using |
|
Are we planning to test LDAP with the simba driver? If yes, then the |
|
We do have Simba specific LDAP tests, but I thought we were only going to run them on Jenkins. |
004b632 to
c2019bf
Compare
|
Everything that can be run on travis should be run on travis. Where's the build on jenkins? In whch branch are the simba-ldap tests? We should check if and how easily we can run those on presto-checks. |
|
Jenkins build is here: https://jenkins-master.td.teradata.com/job/presto-td-product-tests-ldap/ Update: |
|
Do I get it right that the I think those tests should be run in this branch, using the openldap container @sanjay990 prepared. I think we had the container tested? |
|
ldap test group includes CLI and JDBC. It should work with AD or openLDAP. I have tested locally with the openldap container. ldap and ldap_cli both test presto against an LDAP server. Options are: There's not much value in running the JDBC test without the CLI test, so I think only the first two options are actually used. |
|
I have completed all the rebasing. |
|
Great! Let's add the openldap tests though. Otherwise there's nothing that makes sure they work. Do I get it right that ldap-jdbc tests require simba driver? If so, let's run cli_only in this branch and create another one - feature/ldap-simba and base the simba_jdbc branch on feature/ldap-simba |
|
Also, when rebasing the branches, please make sure not to duplicate the commits. cdh_latest-snapshot should base on cdh_latest, and only indireclty on feature/ldap_tests. Sorry for not making this clear in advance |
|
Yes, ldap-jdbc tests do require the simba driver. My understanding is that the Travis LDAP test uses openldap while the Jenkins LDAP test uses AD, thus both are covered. When I rebased the -snapshot branches I did it directly on feature/ldap_tests, and I don't see any duplicate commits in the log...but I'll re-do them using the _latest branches. |
Moving from the regular product-tests run because these tests need extra resources.