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

change spark connection form and add spark connections docs #36419

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

shohamy7
Copy link
Contributor

@shohamy7 shohamy7 commented Dec 25, 2023


Related: #28790
This PR aims to add docs about each Spark connection in a separate page.
In addition, adding a more user-friendly connection form for Spark and Spark SQL connection

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@shohamy7
Copy link
Contributor Author

@Taragolis
Please review, would like to here what you are think on this PR :)

@shohamy7 shohamy7 marked this pull request as draft December 25, 2023 22:21
@shohamy7 shohamy7 marked this pull request as ready for review December 25, 2023 22:32
Copy link
Contributor

@Taragolis Taragolis 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. Just one small NIT from my side

@shohamy7 shohamy7 force-pushed the add-spark-connection-docs branch 2 times, most recently from cfbf318 to 6359a30 Compare December 26, 2023 16:43
@shohamy7 shohamy7 force-pushed the add-spark-connection-docs branch 3 times, most recently from 1bd0bc8 to 274b306 Compare December 27, 2023 08:41
@josh-fell
Copy link
Contributor

@shohamy7 Have you tried to create a connection that isn't one of the connections being updated here? Behind the scenes with connections, all of the connection form data is submitted across all connection types. My concern is that adding the any_of validation might block other connections from being created in the UI because other forms don't have these exact fields.

@shohamy7
Copy link
Contributor Author

@josh-fell
Good point!
I didn't know about this one. I've checked and saw that it is causing issues with other connections.
But I saw that when I add default values, the problem solved.
I saw that we already implement this kind of logic in the VaultHook.

"kv_engine_version": IntegerField(
lazy_gettext("KV engine version"),
validators=[any_of([1, 2])],
widget=BS3TextFieldWidget(),
description="Must be 1 or 2.",
default=DEFAULT_KV_ENGINE_VERSION,
),

I will push a fix with default values.

@shohamy7 shohamy7 force-pushed the add-spark-connection-docs branch from 274b306 to 7edb86b Compare December 27, 2023 22:02
@shohamy7 shohamy7 force-pushed the add-spark-connection-docs branch from 7edb86b to f149819 Compare December 29, 2023 12:34
@eladkal eladkal merged commit ed9080a into apache:main Dec 29, 2023
52 checks passed
@YvesMentens
Copy link

How should we pass the extra params that have been removed in this PR now?

@eladkal
Copy link
Contributor

eladkal commented Mar 25, 2024

How should we pass the extra params that have been removed in this PR now?

It's not removed.
This is just friendlier UI form.

@YvesMentens
Copy link

But how do I pass the extra parameters then? If I cant add them in the UI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants