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.

Sign up for GitHub

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

Jump to bottom

Add UI support for management jobs in workflows #10572

Merged
merged 7 commits into from
Jul 28, 2021
Merged

Add UI support for management jobs in workflows #10572

merged 7 commits into from
Jul 28, 2021

Conversation

mabashian
Copy link
Member

SUMMARY

link #790

This has been supported via the API for a long time. The tricky part was handling the days (days to keep) for some of the management jobs. That value needs to flow in/out of extra_data so it's a bit different from other workflow node fields.

Here it is in action:

mgt_job_wf

I elected to just show the variables in the details view for simplicity. If it's important we could pull days out of the object but more code brings added risk of bugs 🤷 .

Screen Shot 2021-07-01 at 11 23 33 AM

@wenottingham if you're not really into this as a feature that's fine - we can close this and reference it later.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

AlexSCorey
AlexSCorey requested changes Jul 6, 2021
View reviewed changes
Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

Some things I found:

  1. if you type/edit the days field it then shows up as a string in the variable details view after clicking on the i icon when hovering a node. However, when you edit that same node, but don't edit the days field the number appears as an integer in the variables detail after clicking on the i icon. There doesn't seem to be any negative impact on functionality, just some ux inconsistency.

  2. We don't seem to show the convergence data in the detail modal. Should we?

  3. Days vs Days to keep
    Screen Shot 2021-07-06 at 10 19 09 AM
    Screen Shot 2021-07-06 at 10 19 20 AM

  4. Create a Clean up job details node and connect it to a Cleanup expired sessions node. Then click on the Cleanup expired sessions node i icon. Page crashes.
    Screen Shot 2021-07-06 at 10 22 57 AM
    Screen Shot 2021-07-06 at 10 23 08 AM

@mabashian mabashian force-pushed the 790-mgt-jobs-workflow branch from 02d79a4 to aba0340 Compare July 12, 2021 15:20
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@mabashian
Copy link
Member Author

if you type/edit the days field it then shows up as a string in the variable details view after clicking on the i icon when hovering a node. However, when you edit that same node, but don't edit the days field the number appears as an integer in the variables detail after clicking on the i icon. There doesn't seem to be any negative impact on functionality, just some ux inconsistency.

Will include a fix for this

We don't seem to show the convergence data in the detail modal. Should we?

Was probably just missed when we did the initial work. Let's file an issue for this.

Days vs Days to keep

The api expects the key to be days in extra_data so that can't change. Labelling the field Days is ambiguous to me so I don't think we should change it to that. Here's what the field looks like when you launch a management job:

Screen Shot 2021-07-12 at 12 01 24 PM

I'd propose changing the string to something like:

Screen Shot 2021-07-12 at 12 03 23 PM

so that it more closely matches the other interface.

Create a Clean up job details node and connect it to a Cleanup expired sessions node. Then click on the Cleanup expired sessions node i icon. Page crashes.

I'll look at this

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

AlexSCorey
AlexSCorey approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

This looks good to me. The unit tests pass, and the issues described earlier are resolved. I'm triggering e2e tests now.

@tiagodread
Copy link
Contributor

@mabashian could you rebase this one?

@mabashian mabashian force-pushed the 790-mgt-jobs-workflow branch from 8baf4cf to 6935b0e Compare July 22, 2021 18:01
@mabashian mabashian force-pushed the 790-mgt-jobs-workflow branch from 6935b0e to 5f7db08 Compare July 27, 2021 13:03
tiagodread
tiagodread reviewed Jul 27, 2021
View reviewed changes
awx/ui_next/src/util/validators.js
return t`This field must be a number and have a value less than ${max}`;
}
if (!Number.isFinite(max) && value < min) {
return t`This field must be a number and have a value greater than ${min}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is how the ${min} is being displayed.

image

@ansible ansible deleted a comment from cypress bot Jul 27, 2021
@cypress
Copy link

cypress bot commented Jul 27, 2021


Test summary

422 • 0 • 1935 • 0 • Flakiness 0

Run details
Project AWX-PF
Status Passed
Commit 5f7db08
Started Jul 27, 2021 8:12 PM
Ended Jul 27, 2021 9:52 PM
Duration 39:36 💡
OS Linux Debian - 10.2
Browser Chrome 85

View run in Cypress Dashboard ➡️

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

tiagodread
tiagodread approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@tiagodread tiagodread left a comment

Choose a reason for hiding this comment

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

Verified that mgmt jobs can be added as nodes to a workflows and run it, also tested the scheduled workflow with mgmt jobs.

@tiagodread tiagodread mentioned this pull request Jul 28, 2021
Closed
3 tasks
@tiagodread tiagodread merged commit b349774 into ansible:devel Jul 28, 2021
@tiagodread tiagodread mentioned this pull request Jul 28, 2021
Closed
@renovate renovate bot mentioned this pull request Aug 13, 2021
Open
1 task
@nixocio nixocio mentioned this pull request Jun 7, 2022
Closed
6 tasks
nixocio added a commit to nixocio/awx that referenced this pull request Jun 7, 2022
@nixocio
Hide management job for non system admin as node choice
9f49dfe 
Hide management job for non system admin as node type choice. Also, fix
related uni-tests related to this change.

See: ansible#12334
Also: ansible#10572
@nixocio nixocio mentioned this pull request Jun 7, 2022
Merged
InnocentK pushed a commit that referenced this pull request Jun 10, 2022
@nixocio
Hide management job for non system admin as node choice (#12341)
a3e86dc 
Hide management job for non system admin as node type choice. Also, fix
related uni-tests related to this change.

See: #12334
Also: #10572
vedaperi pushed a commit to vedaperi/awx that referenced this pull request Jul 14, 2022
@nixocio
Hide management job for non system admin as node choice (ansible#12341)
cb1d0c0 
Hide management job for non system admin as node type choice. Also, fix
related uni-tests related to this change.

See: ansible#12334
Also: ansible#10572
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiagodread tiagodread tiagodread approved these changes

@AlexSCorey AlexSCorey AlexSCorey approved these changes

Assignees

@tiagodread tiagodread

Labels
component:ui type:enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mabashian @tiagodread @AlexSCorey