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

Do not require all extras for SalesforceHook #19530

Merged
merged 5 commits into from
Nov 12, 2021

Conversation

alexbegg
Copy link
Contributor

@alexbegg alexbegg commented Nov 10, 2021

The SalesforceHook was previously requiring all connection extras, however with secrets backends it is now possible to provide one extra at a time. The current setup was directly accessing keys of the extras json which would result in a KeyError if you leave out one of them.

This PR allows for just the minimum required extras to be provided for each Salesforce connection method as defined in the docstring of the SalesforceHook:

A connection to Salesforce can be created via several authentication options:
* Password: Provide Username, Password, and Security Token
* Direct Session: Provide a `session_id` and either Instance or Instance URL
* OAuth 2.0 JWT: Provide a Consumer Key and either a Private Key or Private Key File Path
* IP Filtering: Provide Username, Password, and an Organization ID

In addition to allowing for only the required extras it also still allows for all of the extras to be provided, such as the case currently when the connection is added via the UI.

When making the connection the hook will explicitly default to None for all extras except for "version" (which defaults to the default API version). The reason for this is because simple-salesforce already has built-in authentication-choosing methods that relies on which arguments are None and without having or None in the code then setting this connection in the UI will result in the blank extras being empty strings instead of None, which would break the connection if get was used on its own.

The tests for the hook are also updated to only test for the required connection params when testing connections and also test that if all extras are empty strings they will be defaulted to None.

Also, a thank you @gigatexal, @dstandish, @josh-fell for prior PRs and PR discussions regarding this fix. This PRs incorporates the discussed points.

closes: #19506
completes PR #18929 by adding tests


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@alexbegg alexbegg changed the title Do not require all extras for SalesforceHook and allow non-prefixed extras Do not require all extras for SalesforceHook Nov 11, 2021
Copy link
Contributor

@dstandish dstandish left a 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, pending the cached_property import fix

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 11, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@alexbegg
Copy link
Contributor Author

Looks good to me, pending the cached_property import fix

The import is fixed

@dstandish
Copy link
Contributor

excellent, thank you

@dstandish dstandish merged commit a24066b into apache:main Nov 12, 2021
@gigatexal
Copy link

Thank you @alexbegg for getting this across the line!

@halilduygulu
Copy link

thanks for the change. in which version this will be released? https://pypi.org/project/apache-airflow-providers-salesforce/#history

@potiuk
Copy link
Member

potiuk commented Nov 17, 2021

In the next one released ~ the end of the month

@halilduygulu
Copy link

and was changing extra parameter names absolutely necessary? I have to change this 40 times in our airflow setup, not everyone is creating connections from ui.

security_token -> "extra__salesforce__security_token" 
domain -> "extra__salesforce__domain"

@dstandish
Copy link
Contributor

I totally feel your pain. I do not like the decision. My very uninformed guess is that more often than not creds are not managed in UI and I would prefer if we never implemented the UI customizations. I think @alexbegg may also have been working on a PR to make it so that you can optionally use the "normal" names instead. But clearly that won't help you in time.

BUT you have an option. Since all the conn extra parsing is done in get_conn you could just subclass the hook and override the behavior so that it looks for the normal names. Then if you are using your own salesforce operator you can just point to your salesforce hook. And if your not you could create a subclass of the salesforce operator that uses your salesforce hook. Depending on your setup this could be an easier path for you.

@alexbegg
Copy link
Contributor Author

alexbegg commented Nov 19, 2021

Yes, I am planning to make a separate PR to have get_conn to allow both formats, security_token and extra__salesforce__security_token. This PR initially allowed for that however I figured it was too much discussion regarding that to be incorporated into this PR.

There is a current discussion on the dev list about why the names changed: https://lists.apache.org/thread/ofwxrx7rcwcn2h6z6m51bfymoqktnbbq

@alexbegg
Copy link
Contributor Author

This PR is using the same extra names as the previous version of the method. Once I make the PR I'll link it here. Also I'm a bit busy at the moment if someone want to handle making a PR in my place for that I'll appreciate that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Salesforce connections should not require all extras
5 participants