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

Edit variable button on variable list page #45251

Merged
merged 16 commits into from
Dec 29, 2024

Conversation

shubhamraj-git
Copy link
Contributor

related: #43709

Note:
This needs to rebase on #45238, once that is merged.

image

^ 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:UI Related to UI/UX. For Frontend Developers. label Dec 27, 2024
@shubhamraj-git
Copy link
Contributor Author

shubhamraj-git commented Dec 27, 2024

@pierrejeambrun @jscheffl @bbovenzi

I can see that the patch variable api have a check

if patch_body.key != variable_key:
        raise HTTPException(
            status.HTTP_400_BAD_REQUEST, "Invalid body, key from request body doesn't match uri parameter"
        )
    non_update_fields = {"key"}

Is this introduced new that we can't edit the variable key?
I checked in the legacy UI and I was able to edit it.

@shubhamraj-git shubhamraj-git changed the title Edit variable Edit variable button on variable list page Dec 27, 2024
Copy link
Contributor

@jscheffl jscheffl 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 for me. LGTM!

(we could debate if it makes sense to add a "ManageVariable" subfolder just for the CRUD elements but actually that is rather cosmetic)

@shubhamraj-git
Copy link
Contributor Author

shubhamraj-git commented Dec 28, 2024

@jscheffl I am open to suggestion on this, if we have something better.
My only take on this was, we are going to have import vars, export var, delete multiple, action bars soon. So, it will contain lot of files, so just made the proper CRUD under a folder.

@jscheffl
Copy link
Contributor

@pierrejeambrun @jscheffl @bbovenzi

I can see that the patch variable api have a check

if patch_body.key != variable_key:
        raise HTTPException(
            status.HTTP_400_BAD_REQUEST, "Invalid body, key from request body doesn't match uri parameter"
        )
    non_update_fields = {"key"}

Is this introduced new that we can't edit the variable key? I checked in the legacy UI and I was able to edit it.

I believe that this is an artefact from the migration of public API from Connexion to Fast API, this validation was converted 1:1.
The legacy UI was not using the public API but was directly writing to DB.

So technically we have two options:

  1. Restrict changing the key in UI, make the key field read-only when editing
  2. Remove the validation in Rest API
  3. Add a UI specific API just for this

From UI Perspective dis-allowing to change the key does not make sense. So I'd propose to remove the validation in REST API. For option 1 I see no reason. And Option 3 is too much overhead just because of a cross-validation in API

Any other opinions?

@shubhamraj-git
Copy link
Contributor Author

@pierrejeambrun @jscheffl @bbovenzi

I can see that the patch variable api have a check

if patch_body.key != variable_key:
        raise HTTPException(
            status.HTTP_400_BAD_REQUEST, "Invalid body, key from request body doesn't match uri parameter"
        )
    non_update_fields = {"key"}

Is this introduced new that we can't edit the variable key? I checked in the legacy UI and I was able to edit it.

Also regrading this issue.

If we plan to implement this.

  1. I can have a PR where I will disable the key edit.
  2. But if we chose to remove this check from backend, I can have a PR to do that.

@shubhamraj-git
Copy link
Contributor Author

shubhamraj-git commented Dec 28, 2024

I believe that this is an artefact from the migration of public API from Connexion to Fast API, this validation was converted 1:1. The legacy UI was not using the public API but was directly writing to DB.

So technically we have two options:

  1. Restrict changing the key in UI, make the key field read-only when editing
  2. Remove the validation in Rest API
  3. Add a UI specific API just for this

From UI Perspective dis-allowing to change the key does not make sense. So I'd propose to remove the validation in REST API. For option 1 I see no reason. And Option 3 is too much overhead just because of a cross-validation in API

Any other opinions?

Regarding option 1)
If we change the key itself, I think we actually created a new variable itself, Isn't this contradicting to edit behaviour? Like users will need to actual change the references too in that case.

I think this can have a discussion and can be fixed later in other PR, what say?

@shubhamraj-git
Copy link
Contributor Author

@potiuk @pierrejeambrun @jscheffl
Based upon the discussion, https://apache-airflow.slack.com/archives/C07813CNKA8/p1735405229925579
I have disabled the variable key edit from UI side.

@jscheffl jscheffl merged commit 7229e2f into apache:main Dec 29, 2024
40 checks passed
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
* initial structure setup

* add editVariable

* initial edit setup

* decouple add variable with form

* edit modal

* fix checks

* rename folder

* bug fix

* reset error

* rename

* rename add

* not edit key
agupta01 pushed a commit to agupta01/airflow that referenced this pull request Jan 6, 2025
* initial structure setup

* add editVariable

* initial edit setup

* decouple add variable with form

* edit modal

* fix checks

* rename folder

* bug fix

* reset error

* rename

* rename add

* not edit key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants