Hide overrides key if empty, display comment for flags if empty #7986

mna · 2022-09-27T20:09:11Z

#7377 (frontend portion)

NOTE: This builds on top of #7979 (contains commits of both). This one is for the frontend only, and to see the diff just for this PR, see here: mna-7377-agent-options-cliflags-2...mna-7377-agent-options-cliflags-3-frontend

Checklist for submitter

If some of the following don't apply, delete the relevant line.

Manual QA for all new/changed functionality

cypress · 2022-09-27T20:54:16Z

Test summary

208 • 0 • 0 • 0 • 0

Run details


Project	Fleet
Status	Passed
Commit	c2e90c12a5 ℹ️
Started	Oct 3, 2022 1:28 PM
Ended	Oct 3, 2022 1:45 PM
Duration	17:22 💡
OS	Linux Ubuntu - 20.04
Browser	Electron 94

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

mna

Hey frontend reviewers 👋 This is my first frontend PR with significant-ish changes, so let me know if I missed anything, especially regarding formatting/conventions/etc. The change seems to work well from manually playing with it locally, but if you feel it's missing anything or could be done a simpler/better way, please let me know, I'm not super familiar with typescript and react (nor with our frontend codebase and tooling!).

This PR is cut off from #7979 and includes those changes (as I had to target the main branch for CI to work properly), but to see just this PR's changes, you can click here: mna-7377-agent-options-cliflags-2...mna-7377-agent-options-cliflags-3-frontend

mna · 2022-09-27T20:17:56Z

frontend/pages/admin/AppSettingsPage/cards/Agents/Agents.tsx

@@ -26,7 +26,7 @@ const Agents = ({
  const { ADMIN_TEAMS } = paths;

  const [formData, setFormData] = useState<any>({
-    agentOptions: yaml.dump(appConfig.agent_options) || {},
+    agentOptions: agentOptionsToYaml(appConfig.agent_options) || {},


I just modified the code that was already there, but shouldn't the || {} be inside the parens? I.e. to yaml-encode an empty object if appConfig.agent_options is null/undefined? Because yaml.dump returns a string, it seems weird to me to change an empty string to an object if the string is empty.

I think you're right, moreover, agentOptions is being assigned to a string below, so it doesn't make sense to assign an object here.

A fun fact about the JS ecosystem (but I don't think it applies here): many libraries have their type definitions separated from their source code, so you can't fully trust them, the return type might say string, but the actual behavior of the library might be undefined (for example) so a case like this one might be intentional

mna · 2022-09-27T20:19:01Z

...tend/pages/admin/TeamManagementPage/TeamDetailsWrapper/AgentOptionsPage/AgentOptionsPage.tsx

@@ -43,7 +43,7 @@ const AgentOptionsPage = ({

        if (selected) {
          setFormData({
-            osquery_options: yaml.dump(selected.agent_options),
+            osquery_options: agentOptionsToYaml(selected.agent_options),


Should we have the same || {} (presumably inside the parens) here too?

if yaml.dump gracefully handles falsey values, I'd be inclined to just leave it like this. Otherwise yes, I vote to add the safety || {}

I ended up moving the check inside agentOptionsToYaml instead.

mna · 2022-09-27T20:20:18Z

frontend/utilities/helpers.ts

@@ -200,6 +200,7 @@ export const formatConfigDataForServer = (config: any): any => {
 };

 // TODO: Finalize interface for config - see frontend\interfaces\config.ts
+// TODO(mna): this appears to be unused?


If we're unsure I can just remove my comment. Never seems to be called from a code search but there may be more to it to be sure.

i'd say remove it. I dont see any use of it in the codebase

roperzh

LGTM, super cool way of handling things. I left a couple of questions

roperzh · 2022-09-28T13:54:32Z

frontend/utilities/yaml/index.ts

  return `${yamlError.name}: ${yamlError.reason} at line ${yamlError.line}`;
 };

+export const agentOptionsToYaml = (agentOpts: any) => {


is there any chance of agentOpts being undefined or null?

Currently this was done (or meant to be done, I guess) at the callsite with the || {} but you're right, maybe it should be done here inside this function instead.

we dont have the best typing in some parts of the app atm. I'd look at where this code is used to determine if that's a possiblity.

Added the check inside the func. Regarding the type, agent_options is not strictly defined even on the backend, it can have arbitrary keys as those are passed to the osquery agent ultimately, so I left it as any.

roperzh · 2022-09-28T14:05:07Z

...tend/pages/admin/TeamManagementPage/TeamDetailsWrapper/AgentOptionsPage/AgentOptionsPage.tsx

@@ -43,7 +43,7 @@ const AgentOptionsPage = ({

        if (selected) {
          setFormData({
-            osquery_options: yaml.dump(selected.agent_options),
+            osquery_options: agentOptionsToYaml(selected.agent_options),


if yaml.dump gracefully handles falsey values, I'd be inclined to just leave it like this. Otherwise yes, I vote to add the safety || {}

server/fleet/agent_options.go

ghernandez345

FE bit looks good

ghernandez345 · 2022-09-28T14:30:37Z

frontend/utilities/helpers.ts

@@ -200,6 +200,7 @@ export const formatConfigDataForServer = (config: any): any => {
 };

 // TODO: Finalize interface for config - see frontend\interfaces\config.ts
+// TODO(mna): this appears to be unused?


i'd say remove it. I dont see any use of it in the codebase

ghernandez345 · 2022-09-28T14:31:28Z

frontend/utilities/yaml/index.ts

  return `${yamlError.name}: ${yamlError.reason} at line ${yamlError.line}`;
 };

+export const agentOptionsToYaml = (agentOpts: any) => {


we dont have the best typing in some parts of the app atm. I'd look at where this code is used to determine if that's a possiblity.

chris-mcgillicuddy · 2022-10-03T20:49:56Z

@gillespi314, could you please review these code changes?

chris-mcgillicuddy

Since there are no copy changes, I asked the on-call engineer to review the code.

mna changed the base branch from mna-7377-agent-options-cliflags-2 to main September 27, 2022 20:28

mna had a problem deploying to Docker Hub September 27, 2022 20:32 Failure

mna temporarily deployed to Docker Hub September 27, 2022 20:36 Inactive

mna commented Sep 27, 2022

View reviewed changes

mna marked this pull request as ready for review September 27, 2022 21:07

mna requested review from ksatter, chris-mcgillicuddy and a team as code owners September 27, 2022 21:07

mna temporarily deployed to Docker Hub September 28, 2022 12:32 Inactive

roperzh previously approved these changes Sep 28, 2022

View reviewed changes

roperzh requested a review from a team September 28, 2022 14:18

mna mentioned this pull request Sep 28, 2022

Add validations for agent options' command-line flags section. #7979

Merged

4 tasks

mna dismissed roperzh’s stale review via a668bac September 28, 2022 18:40

mna force-pushed the mna-7377-agent-options-cliflags-3-frontend branch from 76f1e5f to a668bac Compare September 28, 2022 18:40

mna temporarily deployed to Docker Hub September 28, 2022 18:40 Inactive

ghernandez345 previously approved these changes Sep 29, 2022

View reviewed changes

mna added 3 commits October 3, 2022 08:41

Hide overrides key if empty, display comment for flags if empty

e2ea573

Explicitly type as any

ad913b2

Take safer approach to add the comment to empty command-line flags

4ded49f

mna dismissed ghernandez345’s stale review via 4ded49f October 3, 2022 12:42

mna force-pushed the mna-7377-agent-options-cliflags-3-frontend branch from a668bac to 4ded49f Compare October 3, 2022 12:42

mna temporarily deployed to Docker Hub October 3, 2022 12:42 Inactive

mna added 2 commits October 3, 2022 08:54

Remove unused frontend utility function

03744b6

Cleanup agent opts null/undefined possibility

4767835

mna temporarily deployed to Docker Hub October 3, 2022 13:21 Inactive

mna requested review from roperzh and ghernandez345 October 3, 2022 13:54

chris-mcgillicuddy requested a review from gillespi314 October 3, 2022 20:48

chris-mcgillicuddy reviewed Oct 3, 2022

View reviewed changes

ghernandez345 approved these changes Oct 4, 2022

View reviewed changes

mna merged commit f5b6523 into main Oct 4, 2022

mna deleted the mna-7377-agent-options-cliflags-3-frontend branch October 4, 2022 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide overrides key if empty, display comment for flags if empty #7986

Hide overrides key if empty, display comment for flags if empty #7986

mna commented Sep 27, 2022 •

edited

Loading

cypress bot commented Sep 27, 2022 •

edited

Loading

mna left a comment

mna Sep 27, 2022

roperzh Sep 28, 2022

mna Sep 27, 2022

roperzh Sep 28, 2022

mna Oct 3, 2022

mna Sep 27, 2022

ghernandez345 Sep 28, 2022

roperzh left a comment

roperzh Sep 28, 2022

mna Sep 28, 2022

ghernandez345 Sep 28, 2022

mna Oct 3, 2022

roperzh Sep 28, 2022

ghernandez345 left a comment

ghernandez345 Sep 28, 2022

ghernandez345 Sep 28, 2022

chris-mcgillicuddy commented Oct 3, 2022

chris-mcgillicuddy left a comment

Hide overrides key if empty, display comment for flags if empty #7986

Hide overrides key if empty, display comment for flags if empty #7986

Conversation

mna commented Sep 27, 2022 • edited Loading

Checklist for submitter

cypress bot commented Sep 27, 2022 • edited Loading

Test summary

Run details

mna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roperzh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghernandez345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-mcgillicuddy commented Oct 3, 2022

chris-mcgillicuddy left a comment

Choose a reason for hiding this comment

mna commented Sep 27, 2022 •

edited

Loading

cypress bot commented Sep 27, 2022 •

edited

Loading