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

Update JS packages to latest versions (#9811) #9921

Merged
merged 17 commits into from
Aug 6, 2020

Conversation

retornam
Copy link
Contributor

Signed-off-by: Raymond Etornam [email protected]


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

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jul 22, 2020
@potiuk
Copy link
Member

potiuk commented Jul 22, 2020

You might want to add "Closes #9811 " in the commit message. Did you have a chance to test that the UI works after this upgrade @retornam ? Maybe some more people might test that one before we merge it (the UI is rather weakly tested) Summoning @apache/airflow-committers here.

@potiuk potiuk added this to the Airflow 1.10.12 milestone Jul 22, 2020
@potiuk
Copy link
Member

potiuk commented Jul 22, 2020

Also - even the limited tests here we have did not run because of "test-target" limitation. @retornam - can you please add "^airflow/www" to the patterns in script/ci/tools/ci_check_if_tests_should_be_run.sh to trigger them ?

Update: Ah no - no need to do that. It's another error I see.

@feluelle feluelle self-requested a review July 22, 2020 06:37
"imports-loader": "^0.8.0",
"jquery": ">=3.4.0",
"imports-loader": "^1.1.0",
"jquery": "^3.5.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jquery 3.5.0 previously broke the pause/unpause functionality. Have you tested the webserver after the changes in the PR @retornam ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaxil I started the webserver, logged in but didn't test the example dags. I'll run more detailed testing and fix bugs I find.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@retornam while you are doing the tests, could you document and share with us on what UI functionalities you have tested? That way we can also double check on it with different data or see if any major feature is being missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full details of what I did. I ended up upgrading each package separately. We cannot upgrade the following packages

For UI testing, I checked the following pages

  • /home
    • made sure there were no new JS errors / CSS errors in the console.
    • turned the example dags on and off
    • triggered dags button
    • clicked on tree view button
    • clicked on graph view button
    • clicked on task view button
    • clicked on task tries button
    • clicked on landing times button
    • clicked on the code button
    • clicked on the refresh button
    • clicked on the delete dag button
    • performed a search for a dag using the search button
    • filtered dags using the string example
    • reset the filtered dags
    • clicked on last run for the example_bash operator which I enabled
    • clicked on the successful, failed and running dag links
    • clicked on the recent tasks icon
    • switched tabs between all, active and paused
  • /users/list/
    • added a new user
    • deleted a new user
      -/tree for a dag
    • verified it displays the tree
      -/graph for a dag
    • verified the graph renders
      -/duration for a dag
  • verified the graph renders
    -/tries for a dag
  • verified graph renders
    -/landing_times for a graph
  • verified graph renders
    -/gant for a dag
  • verified graph renders
    -/dag_details for a dag
  • verified page logs
    -/code for a dag
  • verified page loads
  • verified the trigger dag, refresh dag and delete dag buttons work on all pages
  • logged out
  • logged in

I tried to cover as many pages that include the scripts moment.*.js, base.*.js and d3.min.js

Copy link
Member

@feluelle feluelle Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jquery due to a bug with CSRF support in Flask App Builder dpgaspar/Flask-AppBuilder#1362

@kaxil you answered that it was fixed in 2.3.4?! it isn't?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think d3 and nvd3 is not worth the time we need to spend to upgrade the UI.

If we build a new UI from scratch then we should keep that in mind or use a different library then d3.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jquery due to a bug with CSRF support in Flask App Builder dpgaspar/Flask-AppBuilder#1362

@kaxil you answered that it was fixed in 2.3.4?! it isn't?

I believe "fixed" just means "FAB 2.3.4 reverted previous jQuery bump". It's not a fix but just a workaround.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is correct, jQuery 3.5 still causes issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah I am remembering now.

@retornam retornam force-pushed the retornam/fixjs branch 2 times, most recently from f903023 to 88555ab Compare July 23, 2020 21:18
- closes apache#9811
- upgrade webpack and webpack-cli

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- update css-loader to version 3.6.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- update mini-css-extract-plugin to v0.9.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- update lodash to v4.17.19

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- update url-loader to v4.1.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade style-loader to v1.2.1

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade stylelint to v13.6.1

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade handlebars to v4.7.6

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade clean-webpack-plugin to v3.0.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade copy-webpack-plugin to 6.0.3

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade js-yaml to 3.14.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade imports-loader to 1.1.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade file-loader to 6.0.0

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade redoc to 2.0.0-rc.30

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade datatables.net and  datatables.net-bs 1.10.21

Signed-off-by: Raymond Etornam <[email protected]>
- closes apache#9811
- upgrade eslint-plugin-import to 2.22.0
- upgrade eslint to 7.5.0
- babel-eslint to 10.1.0
- babel-loader to 8.1.0
- eslint  to 7.5.0
- eslint-config-airbnb-base to 14.2.0
- eslint-plugin-html to 6.0.2
- eslint-plugin-promise to 4.2.1
- eslint-plugin-node to 11.1.0
- eslint-plugin-standard to 4.0.1

Signed-off-by: Raymond Etornam <[email protected]>
@retornam
Copy link
Contributor Author

retornam commented Aug 4, 2020

@kaxil is anyone else looking at this? Its been a while now I dont want things to go too stale.

@kaxil kaxil added the pinned Protect from Stalebot auto closing label Aug 4, 2020
@kaxil
Copy link
Member

kaxil commented Aug 4, 2020

I plan to include this in 1.10.12 (that would be released next week) so if no-one gets to it first, I will review it soon'ish

@potiuk
Copy link
Member

potiuk commented Aug 5, 2020

I am also happy to do some testing during the weekend before we cut 1.10.12

@fengsi
Copy link

fengsi commented Aug 5, 2020

FYI: CSRF doesn't seem to work for Docker Desktop Kubernetes URL:
http://kubernetes.docker.internal/airflow/

It works on a "real" FQDN. One thing I noticed is the session cookie doesn't exist for kubernetes.docker.internal – could contribute to the CSRF issue. Not sure if it's a browser thing not issuing cookie or due to flask-wtf CSRF internals.

@kaxil
Copy link
Member

kaxil commented Aug 6, 2020

@retornam I still see the following errors in Console on my deployment:
image

@kaxil kaxil merged commit c920b1b into apache:master Aug 6, 2020
@retornam retornam deleted the retornam/fixjs branch August 14, 2020 05:25
potiuk pushed a commit that referenced this pull request Aug 14, 2020
Signed-off-by: Raymond Etornam <[email protected]>
(cherry picked from commit c920b1b)
kaxil pushed a commit that referenced this pull request Aug 15, 2020
Signed-off-by: Raymond Etornam <[email protected]>
(cherry picked from commit c920b1b)
kaxil pushed a commit that referenced this pull request Aug 15, 2020
Signed-off-by: Raymond Etornam <[email protected]>
(cherry picked from commit c920b1b)
kaxil pushed a commit to astronomer/airflow that referenced this pull request Sep 10, 2020
Signed-off-by: Raymond Etornam <[email protected]>
(cherry picked from commit c920b1b)
(cherry picked from commit bce7d1c)
@kaxil
Copy link
Member

kaxil commented Sep 17, 2020

Just want to thank you again @retornam for this PR as it eliminated some CVEs :)

cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Signed-off-by: Raymond Etornam <[email protected]>
(cherry picked from commit c920b1b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues pinned Protect from Stalebot auto closing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants