-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Use new children-summary endpoint data to traverse job event tree #11944
Use new children-summary endpoint data to traverse job event tree #11944
Conversation
8fe3dff
to
4701571
Compare
5abb792
to
27d54b8
Compare
8667c3e
to
280d7f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @keithjgrant, I got this code locally. I did several tests.
I was not able to see the errors reported on the issues. Excellent job.
Also there was a big improvement on the number of requests being made.
Number of requests for job output using awx/devel.
Number of requests for job output using code.
@@ -1,4 +1,3 @@ | |||
/* eslint-disable max-len */ | |||
import React from 'react'; | |||
import { act } from 'react-dom/test-utils'; | |||
import { JobsAPI, JobEventsAPI } from 'api'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove the JobEventsAPI import from this test since we're no longer using it. What are your thoughts on removing the JobEventsAPI from our models completely? It doesn't look like we use that model now that we have this new /jobs/:id/job_events/children_summary/
endpoint.
I triggered the e2e tests. |
Test summaryRun details
View run in Cypress Dashboard ➡️ Failures
Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! The above failed tests are unrelated to this work so it should be fine to merge
Thanks for the fix! Do we know when a new release with this PR will be cut? |
The release cadence of AWX varies, however we try to release a new version of AWX every few weeks. If you feel comfortable running the HEAD of awx/devel then these changes will be immediately available. |
SUMMARY
Updates UI to use the new children-summary endpoint (#11928) to determine number of child events & position in the tree. This greatly reduces the number of network requests made by the job output page for playbook jobs while expand/collapse is enabled.
This also addresses certain job events (such as Warnings) that do not fit into the parent/child tree structure of job events the way normal events do, which was breaking the UI's event lookup algorithm. The problem is these events don't have an
parent_uuid
field even though they might occur between two other events that share a parent. The children-summary endpoint provides a "meta_event_nested_uuid" object to place these events under the correct parent node so the lookup algorithm can work as expected.This depends on #11928 and should not be merged until that PR is also merged.Addresses #11818, #11765, #11647
ISSUE TYPE
COMPONENT NAME