Closed noahtalerman closed 1 year ago
@ghernandez345 I'm assigning you this ticket from the roadmap to begin the spec process. Please spend some time researching the parent epic and related issues to understand goals and context.
Please also review the attached Figma to understand the visual changes. Make sure to read all of the dev notes, as they contain important details.
When you have a clear understanding of how this feature will be implemented, update the "Tasks" list to identify the tasks necessary to complete this issue.
If you have any ideas, questions, or concerns, please tag the appropriate people in a comment on this issue. Thanks!
@ghernandez345 One consideration for this feature is how to order the work between the schema work that Eric is doing and the interface work this ticket covers. Can we build it in unison, or should we wait for the schema work to be finished?
@mike-j-thomas it looks like there's a remaining wireframe TODO. If we still want this tooltip, can you please add this tooltip to the wireframes?
cc @ghernandez345 ^
@noahtalerman @mike-j-thomas what does "anything else if relevant" mean here? are there other OS platforms that could possibly be displayed? which are those and do we have icons we want to show with them?
@noahtalerman @zhumo I think we need to add the data to the fleet schema for columns that default to root in order to implement this on the UI. I dont see anything on the columns
objects that would give us this atm.
@noahtalerman can I get more clarity about what the dev note about replace newlines with spaces means?
@noahtalerman @zhumo looking at the fleet schema we have examples
as plural.
"examples":"// This will get everything\nSELECT * FROM account_policy_data;"
does this mean we could possibly have multiple query examples for a table? if so how do you feel about the data structure looking more like this?
"examples": {
"description": "This will get everything",
"queries": ["SELECT * FROM account_policy_data;", "SELECT * FROM account_policy_data WHERE uid = 1;"]
}
@ghernandez345 One consideration for this feature is how to order the work between the schema work that Eric is doing and the interface work this ticket covers. Can we build it in unison, or should we wait for the schema work to be finished?
@lukeheath i think we can work in parallel when building. as long as we have an agreed contract of how the schema will look and it has all the data we will need. Of course, we'll need the schema implemented to release to prod though
@ghernandez345
can I get more clarity about what the dev note about replace newlines with spaces means?
On the website version, the SQL is easier to view when broken into new lines.
But we don't have much space in the product sidebar, so to reduce scrolling, new lines should be replaced by a space instead. So the SQL will appear wrapped.
I updated the wireframes to better show the wrapped SQL. Sorry for the confusion.
@ghernandez345
can I get more clarity about what the dev note about replace newlines with spaces means?
On the website version, the SQL is easier to view when broken into new lines.
But we don't have much space in the product sidebar, so to reduce scrolling, new lines should be replaced by a space instead. So the SQL will appear wrapped.
ah ok. @noahtalerman @zhumo will the new fleet schema include new line characters within the example queries? in the example query there are none included atm. to display the queries in s smaller space its fine if the queries dont have newline characters
"examples": "// This will get everything\nSELECT * FROM account_policy_data;"
@ghernandez345
I don't believe there are any more OS icons. We pared down to mac, windows, and linux a release or two ago.
Yes, there will be individual attribute defaults to root in the example schema.
The examples attribute is intended to be a blank open text field which accepts markdown. It will/should be joined into a sentence that supports newlines. Do the newlines need to be explicitly written or can it be inferred? @mike-j-thomas @eashaw
@zhumo Newlines should be explicitly written as \n
@ghernandez345 Heads up; you are now unblocked on this. @eashaw hooked us up with a file at https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json that he will be updating every three weeks as part of the digital experience rituals. Let us know if you run into any blockers. Thanks!
@noahtalerman @zhumo @eashaw I'm looking through the schema here https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json and do not see any instances of requires_user_context
on any of the columns. Is this correct? If so can we safely assume columns will never have that property?
Also if this is the case, In the UI you will never see the Defaults to root
messaging in the updated sidebar.
yes, "defaults to root" is
requires_user_context
.
Is this correct or has "defaults to root" been associated with a different column property now?
@eashaw I also notice we use different values for platform names for a table and its columns
e.g
{
// for a table
platforms: [
"darwin",
"windows",
"linux",
],
// for columns
columns: [
{
platforms: [
"macOS",
"Windows",
"Linux",
]
]
}
is it possible that we only use darwin
, windows
, and linux
for consistency? It also leads to less checking on the frontend for additional values.
@noahtalerman there seems to be a new platform type named freebsd
.
{
....
"platforms": [
"darwin",
"linux",
"windows",
"freebsd"
],
....
}
Do we have an icon we want to use to display this in the UI?
Hmm, we don't want to display FreeBSD because osquery no longer supports it. Context is here: #4507
@ghernandez345 where did you see freebsd
show up?
Hmm, we don't want to display FreeBSD because osquery no longer supports it. Context is here: #4507
@ghernandez345 where did you see
freebsd
show up?
In our new osquery_fleet_schema.json. This is the data that will power the website and new sidebar. https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json. @eashaw is there any issue with removing this platform from this schema?
@zhumo @eashaw I noticed that the strings for the examples do not match the proposed schema. They seem to add ```\n characters around the bit that is supposed to be the query.
e.g.
"examples": "See the domain, if any, that the Mac is bound to.\n```\nSELECT domain FROM ad_config;\n```"
This differs from the schema requirements in this issue https://github.com/fleetdm/confidential/issues/1619
"examples":"This will get everything\nSELECT * FROM account_policy_data;", # supports markdown
Is it possible to remove the ```\n characters as the web UI has been built with that string format in mind? Currently, the examples look like this.
.
Somewhat related I also notice that we can have multiple examples for a table, but did not account for that in our schema example. I see from the data in the schema that we've solved this with this set of characters```\n\n. As we are generating this schema ourselves, could we just change it so the data is more intuitive to work with. Something like
{
examples: [
{
description: "description 1",
query: "SELECT * "
},
{
description: "description 2",
query: "SELECT * "
}
],
}
I think not only is it more intuitive but easier to maintain, easier to add more examples, and less likely to lead to bugs from having to parse a complicated string.
@noahtalerman @zhumo @eashaw I'm looking through the schema here https://github.com/fleetdm/fleet/blob/main/schema/osquery_fleet_schema.json and do not see any instances of
requires_user_context
on any of the columns. Is this correct? If so can we safely assume columns will never have that property?Also if this is the case, In the UI you will never see the
Defaults to root
messaging in the updated sidebar.yes, "defaults to root" is
requires_user_context
.Is this correct or has "defaults to root" been associated with a different column property now?
@ghernandez345 This is correct "defaults to root" is requires_user_context
.
is it possible that we only use darwin, windows, and linux for consistency? It also leads to less checking on the frontend for additional values.
@ghernandez345 Yes! The column platform names are changed to the other values in get-extended-osquery-schema.js
to add "Only available on _____ " to column descriptions. I can make a PR to update the behavior to only do this when merging the schema to build Markdown pages.
@zhumo @eashaw I noticed that the strings for the examples do not match the proposed schema. They seem to add ```\n characters around the bit that is supposed to be the query.
e.g.
"examples": "See the domain, if any, that the Mac is bound to.\n```\nSELECT domain FROM ad_config;\n```"
This differs from the schema requirements in this issue fleetdm/confidential#1619
"examples":"This will get everything\nSELECT * FROM account_policy_data;", # supports markdown
Is it possible to remove the ```\n characters as the web UI has been built with that string format in mind? Currently, the examples look like this.
.
Somewhat related I also notice that we can have multiple examples for a table, but did not account for that in our schema example. I see from the data in the schema that we've solved this with this set of characters```\n\n. As we are generating this schema ourselves, could we just change it so the data is more intuitive to work with. Something like
{ examples: [ { description: "description 1", query: "SELECT * " }, { description: "description 2", query: "SELECT * " } ], }
I think not only is it more intuitive but easier to maintain, easier to add more examples, and less likely to lead to bugs from having to parse a complicated string.
@ghernandez345 I think your suggested format makes sense, and it would be relatively easy to update the build script to use it instead of the current string format. @zhumo @GuillaumeRoss What are your thoughts on this change?
@ghernandez345
do not see any instances of requires_user_context on any of the columns. Is this correct?
Sharp eye! requires_user_context
is not a column attribute that is native to osquery. Instead, osquery specifies it by table. the requires_user_context
is manually added in by us in the fleet schema to be column-specific. This way, we can tell the user which column is the relevant one.
@ghernandez345 I would be supportive of always having the platform name be lowercase as you're suggesting, but do you know which source is generating the capitalized one and which source is generating the downcased one?
@ghernandez345 freebsd has been removed from the schema after the next osquery version. When we get a new schema generated, that will naturally disappear from the schema. Up to you guys whether you want to remove it or will just ignore it.
@ghernandez345 @eashaw It has been a huge pain putting in that ```\n\n stuff. However, I would like to try to maintain some amount of flexibility for markdown rather than get locked into the strict "description/example" approach you've proposed. Maybe one step back could be that the examples became an array of strings and the system will auto-join them with \n. So:
{
examples: [
"My description of my query.",
"", # Empty string for an empty line
"```",
"SELECT *",
etc. etc.
]
}
This both preserves flexibility, while making it easier to read.
@zhumo @eashaw , I had a discussion with @lukeheath about some of our options for implementing this UI and since we want to get this out in this release I'm going to change my code to match the current osquery_fleet_schema.json. So no need to make any changes on your side atm.
@eashaw I do think we need to correct the example string for the keychain_acls
. It is missing the \n```\n
characters when separating the description from the example.
{"examples": "Identify keychain items with permissions granted to Applications at the system or user level.SELECT * FROM keychain_acls WHERE path LIKE '/System/Applications/%%' OR path LIKE '/Users/%%/Applications/%%';\n```\nSELECT * FROM keychain_acls WHERE path LIKE '/System/Applications/%%' OR path LIKE '/Users/%%/Applications/%%';\n```"}
@noahtalerman @zhumo @eashaw Ok so I've tried to update my UI from the new data query and I noticed inconsistent data for the examples
attribute that will need to be fixed on the schema generation side. I need the data to be consistent to correctly parse and display this data in the UI. here are a couple of examples.
how most examples are
"examples": "Select all users.\n```\nSELECT * FROM users;\n```"
missing first set of separation characters between description and first query
"examples": "Select all users.SELECT * FROM users;\n```"
missing final separation characters at the end, seems to happen with tables with multiple examples e.g. `account_policy_data
"examples": "Select all users.\n```\nSELECT * FROM users;\n```\nSELECT * FROM users;"
@zhumo heads up, this likely won't make it into the next release (4.22). Code freeze starts EOD Friday (2022-10-14).
My current understanding on what's left:
To wrap up the UI, we need the updates to the schema that @ghernandez345 called out in this comment: https://github.com/fleetdm/fleet/issues/7080#issuecomment-1276674479
@ghernandez345 why do these formatting issues block your work? They are still valid Markdown right? They should still render as long as you feed it into the markdown parser.
A few small items I've found that could use some help. 1) The ordering of the operating systems is out of spec with Figma. The ordering should be macOS, Windows, Linux. 2) Some keys can overrun the space provided and push the data type. 3) When the notes section shows syntax there is an extra line added that should be omitted
Pics:
@noahtalerman @mike-j-thomas For this issue I was thinking we can line break these long column names. What do you think? I tried line break but the UX is annoying as users arent able to copy the whole column name.
before
After
doesn't look amazing but I think this case is rare and has better UX then truncating
I tried line break but the UX is annoying as users arent able to copy the whole column name.
@ghernandez345 did you mean "truncate" instead of "line break" in your message above?
Line breaking the long column names makes sense! I think this is a great solution. Prioritizing the ability to copy makes sense.
The tooltip indicator is a bit awkward for the two line column names. It hangs past the 2nd line. We can address this in a later pass.
cc @mike-j-thomas
@noahtalerman yep, my bad. I tried both truncated and linebreak and linebreak had a better UX imo
Good catch, @ghernandez345. Yeah, truncating would look much better, but if it is hindering copy and pasting, then I agree that line-breaking is the better solution. It does look quite rough though. Is it possible to at least break the line after an "_"? That would help with readability.
Example:
memory_array_mapped_
address_handle
cc @noahtalerman
@ghernandez345, the truncated text interfering with the chevron in the dropdown also needs addressing, albeit a different issue.
Thanks Mike!
@ghernandez345 do you think it makes sense to pull the above fixes into the "User interface polish pass (v1) issue? #8238
This way, they're tracked.
Goal
Update query console right side panel to improve first-time experiences with Fleet and make it more approachable to new users.
Figma
https://www.figma.com/file/yLP0vJ8Ms4GbCoofLwptwS/%E2%9C%85-fleetdm.com-(current%2C-dev-ready)?node-id=5197%3A20730
Requirements
Related
Notes
Tasks