-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(ingest/nifi): kerberos authentication #8097
feat(ingest/nifi): kerberos authentication #8097
Conversation
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.
Mainly concerned about the addition of ruamel.yaml
as a dependency for all sources. Otherwise LGTM
metadata-ingestion/setup.py
Outdated
@@ -57,7 +57,7 @@ def get_long_description(): | |||
"requests_file", | |||
"jsonref", | |||
"jsonschema", | |||
"ruamel.yaml" | |||
"ruamel.yaml", |
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.
What's this for?
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.
formatting with black added this comma.
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.
Oops lol
@@ -112,13 +116,30 @@ class NifiSourceConfig(EnvConfigMixin): | |||
|
|||
# Required to be set if nifi server certificate is not signed by | |||
# root CA trusted by client system, e.g. self-signed certificates | |||
ca_file: Optional[str] = Field( | |||
ca_file: Optional[Union[bool, str]] = Field( |
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.
How is this a bool?
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.
In case, we do not wish to provide the certificate but simply disable the ssl verification, this can be set to False.
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.
Passed as verify
flag on requests api .
rip I think the merge pre-empted itself :( only 1 test was left |
Checklist