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 smart inventory add/edit forms and host filter lookup #7644

Merged
merged 4 commits into from
Aug 10, 2020
Merged

Add smart inventory add/edit forms and host filter lookup #7644

merged 4 commits into from
Aug 10, 2020

Conversation

marshmalien
Copy link
Member

@marshmalien marshmalien commented Jul 17, 2020 •
edited
Loading

SUMMARY

Issues: #7473 #7472

This PR adds the smart inventory add and edit forms, as well as the host filter lookup. A Smart Inventory is a collection of hosts defined by the host filter search. The host filter only applies to hosts of inventories that belong to the Smart Inventory's organization, therefore, the host filter lookup will remain disabled until the organization field is given a value. If a user does not have permission to POST a smart inventory, the form save button will remain disabled.

The smart host filter lookup works a little differently from our other lookups. Instead of selecting hosts from a list, you create a search tag that contains the hosts you want. I created a separate HostFilterUtils file with helper methods to handle converting the query string and host filter string to a searchParams object and back. The host filter lookup doesn't handle advanced search features such as conditional or, compound expressions (), or relational queries using host_filter. The lookup exposes the following filter keys: name, id, groups__name, inventory, instance_id, enabled, last_job, and insights_system_id.

This issue #5513 mentions populating keys based on specific ansible fact names, but I'm not sure it is possible to search on host_filter with our current basic search toolbar. I may be wrong about that assumption, but I wasn't successful at finding a way to prepend host_filter to a namespaced query string
?smart_hosts.host_filter=ansible_facts__ansible_processor_vcpus=8 without breaking the search chip groups. My goal was to get as far as possible without editing the Search component.

Smart inventory form
Screen Shot 2020-07-17 at 3 21 48 PM

Host filter modal
Screen Shot 2020-07-17 at 4 10 05 PM

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

@marshmalien marshmalien added component:ui_next type:feature prioritized on a feature board labels Jul 17, 2020
@marshmalien marshmalien force-pushed the smart-inventory-forms branch from f2146ba to 92e5124 Compare July 20, 2020 17:11
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@mabashian
Copy link
Member

It looks like something's getting a little sideways when searching for a string with a space in it. It looks like the space is getting encoded and then displayed that way. When I open up the host filter modal my matching hosts disappear:

test_smart_inv

@mabashian
Copy link
Member

You may have already talked with Bill/Taufique about this but is the plan for this field to just show the raw string:

Screen Shot 2020-07-22 at 9 15 17 AM

or do we need to somehow convert that in to "search tags" to match:

Screen Shot 2020-07-22 at 9 16 42 AM

@mabashian
Copy link
Member

I don't think we need this prompt on launch - maybe just some copy/paste?

Screen Shot 2020-07-22 at 9 26 18 AM

@mabashian
Copy link
Member

Screen Shot 2020-07-22 at 9 29 39 AM

👍 on making this disabled until the user selects an Org. You may have already discussed this but should we add a tooltip to this field indicating that an Org needs to be selected? We could also hide the field while Org is empty but meh, idk if I like that better or not.

I'm fine leaving it the way it is, just figured I'd ask.

@mabashian
Copy link
Member

I'm a little bit concerned about including this field in the default search values:

Screen Shot 2020-07-22 at 9 37 10 AM

at first I thought that it was the key: ansible_facts. Then I went and looked at the OPTIONS and realized it's The date and time ansible_facts was last modified.. I'm not sure that that's super useful when creating a host filter and may just be confusing to folks that are trying to build a query using ansible_facts.

@mabashian
Copy link
Member

It looks like these come straight from the OPTIONS response:

Screen Shot 2020-07-22 at 9 44 39 AM

but I'm wondering if we should update Inventory -> Inventory ID since it seems like that field requires a number.

Also wondering if we have a pattern for boolean fields like Enabled. This field only really has two options true and false. Rather than forcing them to type it in could we just give them a dropdown with those two options?

This may be bleeding into a larger discussion about what fields we want to make available on the "basic" search level and what we want to make available in "advanced".

@mabashian
Copy link
Member

Outside the scope of this particular PR (though related) is #3427 i.e. UI support for OR logic in the host_filter. This is already supported by the API. The most basic solution to this problem would be to expose a simple text input that a user could opt in to when creating a smart inventory host filter where a complete host_filter string could be supplied. This would serve 2 purposes:

  1. Users wanting to specify complex/non-ui supported host filter strings could do so in the UI and wouldn't have to go through the API browser
  2. If the UI detects a string that we can't parse into consumable search tags then we just show the string in the input.

I wouldn't expect any of this work to be included in this particular PR but it may impact UX decisions so I felt like it was worth bringing up.

@wenottingham thoughts/opinions?

@mabashian
Copy link
Member

This may be related to #7644 (comment) but a filter with multiple name tags seems to be getting wonky as well:

test_smart_inv_2

If I go in and check for matching hosts after saving there are 0 instead of 1 :(

@marshmalien
Copy link
Member Author

marshmalien commented Jul 22, 2020 •
edited
Loading

You may have already talked with Bill/Taufique about this but is the plan for this field to just show the raw string:

Screen Shot 2020-07-22 at 9 15 17 AM

or do we need to somehow convert that in to "search tags" to match:

Screen Shot 2020-07-22 at 9 16 42 AM

I think the plan is to display this using PatternFly LabelGroups and Labels similar to how we display it using ChipGroups and Chips. The PatternFly LabelGroup component is still in the design phase.

@marshmalien marshmalien reopened this Jul 22, 2020
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@wenottingham
Copy link
Contributor

@mabashian having a text input could work. How would we expose it? (I hate an 'advanced' checkbox, but not sure what else it would be.)

@mabashian
Copy link
Member

@wenottingham yea I think this would be exposed in the advanced section. @jlmitch5 is working on that portion separately as far as I understand so we can talk more about it on his side of things. I do think there's a place for something like that in the UI but we'll have to make sure it's a last resort and not something we expect users to use frequently.

@dsesami dsesami self-requested a review August 3, 2020 20:20
@dsesami
Copy link
Contributor

dsesami commented Aug 5, 2020

spoke with @marshmalien about this. some notes:

  1. It might be good to have X's to delete chips with the search filter. Marly has ideas for later improvements to this area, but those don't apply quite yet.
  2. the search filter modal on the add/edit form does not show any info besides names. This might trip up users who want to filter by (for example) group, but can't immediately tell what group the current list of hosts are in.

@marshmalien marshmalien force-pushed the smart-inventory-forms branch from fdb4d3c to ed6bddd Compare August 6, 2020 15:20
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@marshmalien marshmalien mentioned this pull request Aug 6, 2020
Open
@marshmalien
Copy link
Member Author

marshmalien commented Aug 6, 2020 •
edited
Loading

Will address adding a host filter string text input in this issue: #7850
and expanding host list content in this issue: #7853

@dsesami
Copy link
Contributor

dsesami commented Aug 6, 2020

spoke with @marshmalien. We'll probably get into the ideas for expanded views in a future issue or as possible parts of #7850

@marshmalien marshmalien mentioned this pull request Aug 6, 2020
Open
@marshmalien marshmalien force-pushed the smart-inventory-forms branch from ed6bddd to af218aa Compare August 10, 2020 18:33
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit ddb6c5d into ansible:devel Aug 10, 2020
This was referenced Aug 10, 2020
Closed
Closed
@marshmalien marshmalien mentioned this pull request Oct 28, 2020
Closed
3 tasks
@unlikelyzero unlikelyzero mentioned this pull request Dec 13, 2020
Open
@nixocio
Copy link
Contributor

nixocio commented Nov 12, 2021

Related: #11017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsesami dsesami dsesami approved these changes

@mabashian mabashian mabashian approved these changes

@jlmitch5 jlmitch5 Awaiting requested review from jlmitch5

@chrismeyersfsu chrismeyersfsu Awaiting requested review from chrismeyersfsu

Assignees
No one assigned
Labels
type:feature prioritized on a feature board
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marshmalien @mabashian @wenottingham @dsesami @nixocio