Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix problem with inability to remove fields from Connection form #40421

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

MaksYermak
Copy link
Contributor

In current PR I've fixed a problem with inability to remove fields from Connection form. After changing in Connection's form validation, which was made in this PR #23241 , users can't remove value from fields in Connection form. Each time when users try to remove the value and save changes, and then open this Connection's form one more time the value reverts as if nothing changed.


^ 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.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jun 25, 2024
@eladkal eladkal requested a review from josh-fell June 26, 2024 04:26
@VladaZakharova
Copy link
Contributor

Hi @josh-fell !
Can you please check changes here? Thanks!

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 17, 2024
@MaksYermak MaksYermak force-pushed the fix-edit-connection-form branch from 145126e to 8e9c076 Compare August 19, 2024 15:04
@potiuk
Copy link
Member

potiuk commented Aug 19, 2024

I am not sure what exactly problem with if != "" was (because it is slightly not precise - and the exact scemario described in #23241 does not explain it in detail - but I guess if we won't check it, we can accidentally modify fields when they are removed between the connections.

I tried to follow the scenario described in #23241 but I could not see anything wrong with it.

This whole Connection editing form will have to be recreated in Airflow 3 - and I just realized that just few days ago #41144 has been merged that will show the warning in 2.10.1 that in order to remove values you need to recreate the connection - while not perfect - I would hesitate to merge this one until we are sure what was the reason and scenario we wanted to protect (@josh-fell @dstandish - maybe you will be able to dig it out - I know it's 2 years old, but there was a discussion about the if and it apparently made sense then).

@github-actions github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 20, 2024
@VladaZakharova
Copy link
Contributor

hi @josh-fell @dstandish!
Please check the changes here in PR, thanks!

@VladaZakharova
Copy link
Contributor

Hi @potiuk ! Maybe there is someone else who can also review changes here? I think it can take some time for @josh-fell @dstandish to review it...

@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

@josh-fell @dstandish -> do you recall the scenario this please?

@VladaZakharova
Copy link
Contributor

hi! @josh-fell @dstandish can you please check this one?

@MaksYermak
Copy link
Contributor Author

Hi @josh-fell @dstandish could you please look on this PR?

@potiuk
Copy link
Member

potiuk commented Oct 2, 2024

@josh-fell @dstandish -> any comments? This one seems like a good candidate for a bug fix to be backported to 2.10, as long as we somehow figure out if the edge case vaguely explained in the comments is not affected.

@potiuk
Copy link
Member

potiuk commented Oct 23, 2024

@VladaZakharova @MaksYermak -> I see we have no feedback here, but my question is - how eager you are to get this merged? There are some risks involved if we release it in 2.10.3 - because we want to really avoid relasing 2.10.4 quickly. On the other hand the whole mechanism of connection UI management will be rewritten from scratch in Airflow 3. And there are workarounds for the problem (delete and recreate the connection).

So without having any clue from @josh-fell and @dstandish (they likely don't remember either, though it would be nice to hear back) about the actual problem that introduced this "limitation" - I'd say it's a bit risky to merge that one and backport it for 2.10.3.

Is there an important reason why we want to get this case fixed?

@MaksYermak
Copy link
Contributor Author

@VladaZakharova @MaksYermak -> I see we have no feedback here, but my question is - how eager you are to get this merged? There are some risks involved if we release it in 2.10.3 - because we want to really avoid relasing 2.10.4 quickly. On the other hand the whole mechanism of connection UI management will be rewritten from scratch in Airflow 3. And there are workarounds for the problem (delete and recreate the connection).

So without having any clue from @josh-fell and @dstandish (they likely don't remember either, though it would be nice to hear back) about the actual problem that introduced this "limitation" - I'd say it's a bit risky to merge that one and backport it for 2.10.3.

Is there an important reason why we want to get this case fixed?

Hello @potiuk
In Composer we have a customer issue related to this problem. Also I have one more solution for this problem which is to save current behavior and delete value if the user decides to delete it on UI. Here's the code:

if value != "":
    extra[field_name] = value
elif field_name in extra:
    del extra[field_name]

In my opinion this solution is not risky, because the current behavior is saved. And if we don't have any answer from contributors we can use this solution as a safety one for fixing this problem. WDYT?

@potiuk
Copy link
Member

potiuk commented Oct 31, 2024

Hard to say - but - can you propose the customer to delete and recreate the. connection

@MaksYermak MaksYermak force-pushed the fix-edit-connection-form branch from 8e9c076 to 916a784 Compare November 12, 2024 18:15
@MaksYermak MaksYermak requested a review from jscheffl as a code owner November 12, 2024 18:15
@potiuk potiuk added the backport-to-v2-10-test Mark PR with this label to backport to v2-10-test branch label Nov 28, 2024
@potiuk potiuk added this to the Airflow 2.10.4 milestone Nov 28, 2024
@potiuk potiuk merged commit 14bfe39 into apache:main Nov 28, 2024
53 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 28, 2024
…ion form (#40421)

(cherry picked from commit 14bfe39)

Co-authored-by: Maksim <maksimy@google.com>
Copy link

Backport successfully created: v2-10-test

Status Branch Result
v2-10-test PR Link

potiuk pushed a commit that referenced this pull request Nov 28, 2024
…ion form (#40421) (#44442)

(cherry picked from commit 14bfe39)

Co-authored-by: Maksim <maksimy@google.com>
prabhusneha pushed a commit to astronomer/airflow that referenced this pull request Nov 28, 2024
@utkarsharma2 utkarsharma2 added the type:bug-fix Changelog: Bug Fixes label Dec 4, 2024
utkarsharma2 pushed a commit that referenced this pull request Dec 4, 2024
…ion form (#40421) (#44442)

(cherry picked from commit 14bfe39)

Co-authored-by: Maksim <maksimy@google.com>
utkarsharma2 pushed a commit that referenced this pull request Dec 9, 2024
…ion form (#40421) (#44442)

(cherry picked from commit 14bfe39)

Co-authored-by: Maksim <maksimy@google.com>
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:webserver Webserver related Issues backport-to-v2-10-test Mark PR with this label to backport to v2-10-test branch type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants