Skip to content

Add user comment to task instance and dag run#26457

Merged
bbovenzi merged 28 commits into
apache:mainfrom
Jorricks:introduce-user-comments
Nov 16, 2022
Merged

Add user comment to task instance and dag run#26457
bbovenzi merged 28 commits into
apache:mainfrom
Jorricks:introduce-user-comments

Conversation

@Jorricks

@Jorricks Jorricks commented Sep 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Implement the support for notes inside Airflow.

Quick showcase:
http://www.giphy.com/gifs/RU8JL1GbYdSic9WiU2
Giphy

Closes: #16790

Steps:

  1. Add column to database schema.
  2. Add code inside DagRun & TaskInstance.
  3. Add code to DagRun and TaskInstance views so you can edit it there.
  4. Add code inside API to set Task & DagRun notes.
  5. Add react code to set the DagRun notes.
  6. Make it look & work somewhat decently in the react front-end.
  7. Add react code to set the TaskInstances notes.
  8. Add minor overlay over DagRuns to indicate there is a note or not.
  9. Add minor overlay over TaskInstance to indicate there is a note or not.
  10. Make it work with Mapped Instances as well.
  11. Implemented autofocus.
  12. Implemented accordion to be initially open when there is a note and closed otherwise.
  13. Fix all broken tests
  14. Added notes support in TriggerDAGRunOperator
  15. Add tests for setting notes on DagRun & TaskInstances.
  16. Change UserNotes to notes.
  17. Create collapsable user notes.
  18. Change TaskInstance API endpoint to Patch.
  19. Update names of variables.
  20. Change corner color to transparent over black.

mik-laj
mik-laj reviewed Sep 18, 2022
View reviewed changes
Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
@bbovenzi

bbovenzi commented Sep 23, 2022

Copy link
Copy Markdown
Contributor

I would vote to just call it notes instead of user_notes.
Some variables start becoming quite verbose like useSetDagRunUserNote

@pierrejeambrun

pierrejeambrun commented Sep 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Maybe we could use something similar to what we have for dag documentation ?
image

Keeps UI consistent and make sense for a 'note' ? (Not sure if we need this collapsible though, having the collapsible feature seems nice as the note can be pretty long Column(String(1000)))

@Jorricks

Jorricks commented Sep 24, 2022

Copy link
Copy Markdown
Contributor Author

Maybe we could use something similar to what we have for dag documentation ? image

Keeps UI consistent and make sense for a 'note' ? (Not sure if we need this collapsible though, having the collapsible feature seems nice as the note can be pretty long Column(String(1000)))

Yes good idea! Let's do that.

@Jorricks

Jorricks commented Sep 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I am a bit wondering..
Would it be better to create a separate tab for the TaskInstances or would it be better to also use an accordion in-between clearing/changing and details?
IMO it's better to keep the same tab as we want notes to be the first thing people see, (because if they wouldn't be important, they wouldn't be set).

Edit: Went for collapsible :)

image

@Jorricks

Jorricks commented Sep 25, 2022

Copy link
Copy Markdown
Contributor Author

I modified the look and feel for the TaskInstance and DagRun overview a bit so that the titles have the same font.
Furthermore, also implemented the TaskInstance stuff.
As far as I can judge, this is pretty much ready in terms of functionality.
Just needs a proper code review & some tests.

Latest version can be seen here:
http://www.giphy.com/gifs/FicsjJDKZ7pbDiVFOR

@Jorricks Jorricks mentioned this pull request Sep 26, 2022
Closed
2 tasks
@bbovenzi

bbovenzi commented Sep 26, 2022

Copy link
Copy Markdown
Contributor

Great work with the edit button to change from a paragraph to a textarea! I agree, the accordion is better than a separate tab.

bbovenzi
bbovenzi reviewed Sep 26, 2022
View reviewed changes
Comment thread airflow/www/static/js/dag/grid/dagRuns/Bar.tsx Outdated
bbovenzi
bbovenzi reviewed Sep 26, 2022
View reviewed changes
Comment thread airflow/www/static/js/dag/details/taskInstance/index.tsx Outdated
bbovenzi
bbovenzi reviewed Sep 26, 2022
View reviewed changes
Comment thread airflow/www/static/js/api/useSetDagRunNotes.ts Outdated
bbovenzi
bbovenzi reviewed Sep 26, 2022
View reviewed changes
Comment thread airflow/www/static/js/api/useSetTaskInstanceNotes.ts Outdated
bbovenzi
bbovenzi reviewed Sep 27, 2022
View reviewed changes
Comment thread airflow/www/static/js/dag/details/SetDagTaskNotes.jsx Outdated
bbovenzi
bbovenzi reviewed Sep 27, 2022
View reviewed changes
Comment thread airflow/www/static/js/api/useSetDagRunNotes.ts Outdated
bbovenzi
bbovenzi reviewed Sep 27, 2022
View reviewed changes
Comment thread airflow/www/static/js/dag/details/taskInstance/Details.tsx Outdated
bbovenzi
bbovenzi reviewed Sep 27, 2022
View reviewed changes
Comment thread airflow/www/static/js/dag/details/taskInstance/MappedInstances.tsx Outdated
bbovenzi
bbovenzi reviewed Sep 27, 2022
View reviewed changes
Comment thread airflow/www/static/js/dag/grid/renderTaskRows.tsx Outdated
@Jorricks

Jorricks commented Sep 27, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

To fix in a separate MR;
Rebasing fixed all errors that were present.

@bbovenzi

bbovenzi commented Sep 27, 2022

Copy link
Copy Markdown
Contributor

http://localhost:28080/api/v1/dags/simple_mapping/dagRuns/scheduled__2022-03-04T00:00:00+00:00/taskInstances/add_one

I can't replicate this on main. Could you rebase first?

@Jorricks

Jorricks commented Sep 27, 2022

Copy link
Copy Markdown
Contributor Author

http://localhost:28080/api/v1/dags/simple_mapping/dagRuns/scheduled__2022-03-04T00:00:00+00:00/taskInstances/add_one

I can't replicate this on main. Could you rebase first?

Trying that now.

Jorricks and others added 16 commits November 15, 2022 14:48
@Jorricks

Jorricks commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

Sorry for the closing of the ticket. Happened due to me accidentally pressing 'sync fork', which removed all commits.
I rebased everything. Just checking now that it all still works. Then I'll see if the AWS action change is still present.

@Jorricks

Jorricks commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

Reopen

@Jorricks Jorricks reopened this Nov 15, 2022
@Jorricks Jorricks closed this Nov 15, 2022
@Jorricks Jorricks reopened this Nov 15, 2022
@Jorricks

Jorricks commented Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

Updated the GIF to reflect the latest state

@bbovenzi
bbovenzi merged commit dc03b90 into apache:main Nov 16, 2022
@bbovenzi
bbovenzi deleted the introduce-user-comments branch November 16, 2022 15:29
@Jorricks

Jorricks commented Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

Very happy to get this merged! 🎉 🎉
Thanks everyone for all the feedback and in special @bbovenzi who did major improvements on the Typescript part and helped me out with coding in react for the first time 👍 .

This was referenced Nov 16, 2022
Closed
Closed
@tirkarthi tirkarthi mentioned this pull request Nov 21, 2022
Closed
2 tasks
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashb ashb ashb left review comments
@ephraimbuddy ephraimbuddy ephraimbuddy left review comments
@bbovenzi bbovenzi bbovenzi approved these changes
@mik-laj mik-laj mik-laj left review comments
@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes
@potiuk potiuk Awaiting requested review from potiuk
@kaxil kaxil Awaiting requested review from kaxil
@XD-DENG XD-DENG Awaiting requested review from XD-DENG
@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

Assignees

No one assigned

Labels

area:dev-tools kind:documentation type:new-feature Changelog: New Features

Projects

None yet

Milestone

Airflow 2.5.0

Development

Successfully merging this pull request may close these issues.

Support manual task annotation.

9 participants

@Jorricks @bbovenzi @pierrejeambrun @gbonazzoli @potiuk @ashb @ephraimbuddy @mik-laj @eladkal