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(cli) CA Cert option for CLI commands #3964

Closed
wants to merge 2 commits into from
Closed

fix(cli) CA Cert option for CLI commands #3964

wants to merge 2 commits into from

Conversation

xiphl
Copy link
Contributor

@xiphl xiphl commented Jan 25, 2022

When running CLI commands against a remote instance of datahub, there may be SSL related errors when connecting to the GMS.
While there is an option for ingestion (specify in the recipe), there should be an option for the user to define the CA cert when interacting with the remote instance for the other commands. Basically this PR adds ca_cert for requests.session().

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@github-actions
Copy link

Unit Test Results (build & test)

  45 files  ±0    45 suites  ±0   12m 24s ⏱️ + 1m 4s
505 tests ±0  449 ✔️ ±0  56 💤 ±0  0 ±0 

Results for commit 2b9548a. ± Comparison against base commit f2e2a4d.

@xiphl xiphl changed the title [Bug] CA Cert option for CLI commands fix(cli) CA Cert option for CLI commands Jan 25, 2022
@github-actions
Copy link

Unit Test Results (metadata ingestion)

       5 files  ±0         5 suites  ±0   38m 56s ⏱️ + 3m 28s
   276 tests ±0     266 ✔️ ±0    6 💤 ±0  2 ±0  2 🔥 ±0 
1 243 runs   - 4  1 203 ✔️  - 2  28 💤  - 6  6 +2  6 🔥 +2 

For more details on these failures and errors, see this check.

Results for commit 2b9548a. ± Comparison against base commit f2e2a4d.

@xiphl
Copy link
Contributor Author

xiphl commented Jan 25, 2022

Erm, it seems as though the failure is related to #3961 ?

@shirshanka
Copy link
Contributor

Thanks for the PR @xiphl. The configuration and code differences between cli_utils and rest_emitter is not something we want to continue encouraging. If you can hold off on us merging this PR, we will probably have a simplification to cli-utils coming soon where we will unify the underlying classes (and the resulting config) to communicate with DataHub. Let us know if that works for you.

@xiphl
Copy link
Contributor Author

xiphl commented Jan 26, 2022

I agree that it makes more sense to have a single config for emitter and cli, so I look forward to the revised implementation.

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.

2 participants