Skip to content

Adds support for Netskope on MacOS#88

Open
kaincenteno wants to merge 4 commits into
macadmins:mainfrom
kaincenteno:netskope
Open

Adds support for Netskope on MacOS#88
kaincenteno wants to merge 4 commits into
macadmins:mainfrom
kaincenteno:netskope

Conversation

@kaincenteno
Copy link
Copy Markdown

Runs nsdiag -f and parses the output. nsdiag is installed with netskope

kaincenteno and others added 2 commits May 13, 2026 15:13
Runs `nsdiag -f` and parses the output. nsdiag is installed with netskope
@grahamgilbert
Copy link
Copy Markdown
Member

Thoughts on adding windows and Linux support?

@grahamgilbert
Copy link
Copy Markdown
Member

The implementation looks reasonable overall, but I think the unit tests could be strengthened quite a bit.

A lot of the current coverage is focused on implementation details and happy-path behavior. For example, TestNetskopeColumns and parts of TestKeyToColumn are effectively mirroring the production code, which makes them relatively expensive to maintain without adding much protection against regressions.

The bigger gap is parser robustness. Since this table is consuming human-readable output from an external CLI (nsdiag -f), I think the tests should lean more heavily into edge cases and unexpected input. Right now the parser is only exercised against a single well-formed sample output.

It would be useful to add coverage for things like:

  • malformed lines
  • missing separators
  • duplicate keys
  • empty values
  • extra whitespace
  • values containing ::
  • unknown/future fields
  • partial output
  • mixed warning/debug output

I’d also consider whether trimming trailing . characters from all values is something we want to treat as part of the contract long term, since the current tests implicitly lock that behavior in.

Overall I think the implementation is a good start, but the tests currently give more confidence in the happy path than in the parser’s resilience to real-world output changes.

@kaincenteno
Copy link
Copy Markdown
Author

The implementation looks reasonable overall, but I think the unit tests could be strengthened quite a bit.

A lot of the current coverage is focused on implementation details and happy-path behavior. For example, TestNetskopeColumns and parts of TestKeyToColumn are effectively mirroring the production code, which makes them relatively expensive to maintain without adding much protection against regressions.

The bigger gap is parser robustness. Since this table is consuming human-readable output from an external CLI (nsdiag -f), I think the tests should lean more heavily into edge cases and unexpected input. Right now the parser is only exercised against a single well-formed sample output.

It would be useful to add coverage for things like:

  • malformed lines
  • missing separators
  • duplicate keys
  • empty values
  • extra whitespace
  • values containing ::
  • unknown/future fields
  • partial output
  • mixed warning/debug output

I’d also consider whether trimming trailing . characters from all values is something we want to treat as part of the contract long term, since the current tests implicitly lock that behavior in.

Overall I think the implementation is a good start, but the tests currently give more confidence in the happy path than in the parser’s resilience to real-world output changes.

thanks for the feedback. i have submitted the changes. Let me know if something got missed or misunderstood

@kaincenteno
Copy link
Copy Markdown
Author

Thoughts on adding windows and Linux support?

I can work on that for a subsequent PR 👍 (at least for windows the most immediate).

would it be possible to focus only on macOS for this version or are 3 main OSs required when adding new features?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants