-
Notifications
You must be signed in to change notification settings - Fork 0
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
task/WP-288: Implement Queue Filter #883
Conversation
Codecov Report
@@ Coverage Diff @@
## main #883 +/- ##
=======================================
Coverage 63.32% 63.33%
=======================================
Files 428 428
Lines 12243 12246 +3
Branches 2514 2518 +4
=======================================
+ Hits 7753 7756 +3
Misses 4284 4284
Partials 206 206
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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, thanks for adding this feature. And good to see tests added too. Tested the following:
- app forms with and without queue filter
- use queue filter based to adjust default and submit job
- use app without queue filter and submit job.
One minor change, can you remove queueFilter in job submission payload, if it exists ? At that point it is not needed, exec queue is already populated.
See payload below:
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.
Works well, one change request in addition to Chandra's comment
@@ -44,6 +44,7 @@ const appShape = PropTypes.shape({ | |||
coresPerNode: PropTypes.number, | |||
maxMinutes: PropTypes.number, | |||
tags: PropTypes.arrayOf(PropTypes.string), | |||
queueFilter: PropTypes.arrayOf(PropTypes.string), |
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.
I think we can drop these changes on lines 47 and 272, as these don't correspond to default fields on the form
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.
Yep this change addresses Chandra's comment, in regard to getting rid of 47 and 272 gets rid of queueFilter showing up in the job submission payload.
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.
LGTM! Thanks for addressing comments.
Overview
We know some apps only work on certain queues, and for these apps we want to be able to control which queues show up for the user in the queue selection dropdown.
Direction
Develop a queue filter schema based on a flag in the app's notes field, and implement this on the frontend. I suggest something along the lines of app -> notes -> queueFilter -> [], and if there is no queueFilter flag (or nothing in the array), show all queues
Reference
Talk to Sal for details on the schema for the queue filter in an app definition
Related
Changes
Testing
UI
Notes
This was designed so that in the future when we update other applications or create new applications, we can include queueFilter property in app.definition.notes and further filter which queues we want the users to see on the frontend.
*Frontera system is currently undergoing maintenance, so might have to wait to test dropdown.