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

[Async]HamiltonTracker support passing in custom CA cert #1105

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

flavour
Copy link
Contributor

@flavour flavour commented Aug 26, 2024

Our Hamilton UI server is fronted by an SSL using an internal CA, which isn't supported by Hamilton SDK without this PR.

The standard synchronous HamiltonTracker can be worked around by using os.environ["REQUESTS_CA_BUNDLE"] but that's not ideal. The AsyncHamiltonTracker has no such workaround available & we are switching to using that.

Changes

Trackers take a new optional verify arg which mimics the requests arg of the same name.

How I tested this

Both adapters tested on our internal network

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

@elijahbenizzy any thoughts? I think this look good.

@elijahbenizzy
Copy link
Collaborator

@elijahbenizzy any thoughts? I think this look good.

Yep, looks good, maybe a few more comments on this (it's pretty clear, just the true/false/str is a bit confusing.

@skrawcz skrawcz merged commit 737a3e0 into DAGWorks-Inc:main Aug 28, 2024
27 checks passed
@skrawcz
Copy link
Collaborator

skrawcz commented Aug 28, 2024

@flavour plan is to release the SDK today 🤞 .

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.

3 participants