feat: Dry run flag to print preflight specs to std out #1240

banjoh · 2023-06-23T11:08:56Z

Description, Motivation and Context

Introduce preflight --dry-run flag used to print specs to stdout. I went for --dry-run flag instead of adding a new subcommand (dump, print, view etc) cause it felt a bit more intuitive. It also makes sense since when we pass in multiple specs, --dry-run would print out what would be run in spec format.

Notes for reviewer

Introduce ToYaml() method that marshals TroubleshootKinds to yaml documents
Move cmd/util to cmd/internal/util. Its private implementation
Remove PreflightSpecs in favour of TroubleshootKinds

Partially addresses #1024. We still need to add support-bundle --dry-run flag

Checklist

New and existing tests pass locally with introduced changes.
Tests for the changes have been added (for bug fixes / features)
The commit message(s) are informative and highlight any breaking changes
Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

Yes
No

CLAassistant · 2023-08-24T13:07:23Z

All committers have signed the CLA.

* Remove duplicate version.go files

banjoh · 2023-08-31T18:30:01Z

Here is an example parsing specs from a helm chart (stdin), a url and a local file, then printing to stdout.

[evans] $ helm template examples/sdk/helm-template/mychart-0.1.0.tgz --set preflight=true | ./bin/preflight --dry-run https://raw.githubusercontent.com/replicatedhq/troubleshoot/main/examples/preflight/host/cpu.yaml -
apiVersion: troubleshoot.sh/v1beta2
kind: HostPreflight
metadata:
  creationTimestamp: null
  name: cpu
spec:
  analyzers:
  - cpu:
      outcomes:
      - fail:
          message: At least 4 physical CPU cores are required
          when: physical < 4
      - fail:
          message: At least 8 CPU cores are required
          when: logical < 8
      - warn:
          message: At least 16 CPU cores preferred
          when: count < 16
      - pass:
          message: This server has sufficient CPU cores
  collectors:
  - cpu: {}
status: {}
---
apiVersion: troubleshoot.sh/v1beta2
kind: Preflight
metadata:
  creationTimestamp: null
  name: sample
spec:
  analyzers:
  - postgres:
      checkName: Must be postgres 10.x or later
      collectorName: pg
      outcomes:
      - fail:
          message: Cannot connect to postgres server
          when: connected == false
      - fail:
          message: The postgres server must be at least version 10
          when: version < 10.x
      - pass:
          message: The postgres connection checks out
  collectors:
  - postgres:
      collectorName: pg
      uri: postgresql://user:password@hostname:5432/defaultdb?sslmode=require
status: {}

xavpaice

like the approach, couple of minor questions.

xavpaice · 2023-09-04T01:12:27Z

cmd/preflight/cli/root.go

 	cmd.AddCommand(OciFetchCmd())
 	preflight.AddFlags(cmd.PersistentFlags())

+	// Dry run flag should not persist between across subcommands. Adding here to avoid that
+	cmd.Flags().Bool("dry-run", false, "print the preflight spec without running preflight checks")


isn't this added below in pkg/preflight/flags.go? Not sure I understand the comment.

isn't this added below in pkg/preflight/flags.go? Not sure I understand the comment.

All flags in pkg/preflight/flags.go are also available to all subcommands e.g preflight completions.

[evans] $ preflight completion -h Generate the autocompletion script for preflight for the specified shell. See each sub-command's help for details on how to use the generated script. Usage: preflight completion [command] Available Commands: bash Generate the autocompletion script for bash fish Generate the autocompletion script for fish powershell Generate the autocompletion script for powershell zsh Generate the autocompletion script for zsh Flags: -h, --help help for completion Global Flags: --collect-without-permissions always run preflight checks even if some require permissions that preflight does not have (default true) --collector-image string the full name of the collector image to use --collector-pullpolicy string the pull policy of the collector image --cpuprofile string File path to write cpu profiling data --debug enable debug logging --format string output format, one of human, json, yaml. only used when interactive is set to false (default "human") --interactive interactive preflights (default true) --memprofile string File path to write memory profiling data -o, --output string specify the output file path for the preflight checks --selector string selector (label query) to filter remote collection nodes on. --since string force pod logs collectors to return logs newer than a relative duration like 5s, 2m, or 3h. --since-time string force pod logs collectors to return logs after a specific date (RFC3339) Use "preflight completion [command] --help" for more information about a command.

Running preflight completions --dry-run or preflight completions --since 3h is redundant.

Alternatively, we could change this line to preflight.AddFlags(cmd.Flags())

Personally I prefer each subcommand to explicitly list all the flags it needs to expose like the support-bundle binary does. Thats for another PR though. I thought refactoring this part would have been scope creep.

xavpaice · 2023-09-04T01:16:19Z

pkg/preflight/run.go


+	warning := validatePreflight(specs)
 	if warning != nil {
 		fmt.Println(warning.Warning())
 		return nil


I kinda want this warning in the dry-run also

xavpaice · 2023-09-04T01:19:35Z

pkg/preflight/validate_specs.go

 		if warning != nil {
 			return warning
 		}
 	}

-	if specs.UploadResultSpecs != nil {


can we avoid losing the validation of this type?

can we avoid losing the validation of this type?

TL;DR - We aren't losing the validation. It takes place here where that list of troubleshootv1beta2.Preflight objects will contain both PreflightSpec and UploadResultSpecs preflight objects which were in the original PreflightSpecs struct

Longer description
Changes in this PR have gotten rid of the PreflightSpecs struct in favour of using TroubleshootKinds.

Here is a comparison of the structs

type PreflightSpecs struct { PreflightSpec *troubleshootv1beta2.Preflight HostPreflightSpec *troubleshootv1beta2.HostPreflight UploadResultSpecs []*troubleshootv1beta2.Preflight }

type TroubleshootKinds struct { // redacted ... HostPreflightsV1Beta2 []troubleshootv1beta2.HostPreflight PreflightsV1Beta2 []troubleshootv1beta2.Preflight }

As you can see both PreflightSpec and UploadResultSpecs store *troubleshootv1beta2.Preflight. I changed the logic avoid transforming TroubleshootKinds to PreflightSpecs in favour of just using TroubleshootKinds in an effort of having a common type that houses troubleshoot spec objects. Now objects there were in PreflightSpecs.PreflightSpec and PreflightSpecs.UploadResultSpecs will be in TroubleshootKinds.PreflightsV1Beta2. The difference between objects that were in PreflightSpec and UploadResultSpecs is the presence/absence of Spec.UploadResultsTo which now handled here

Changes can be found in

Validation code

Code to read spec inputs

banjoh added the type::feature New feature or request label Jun 23, 2023

banjoh requested a review from a team as a code owner June 23, 2023 11:08

banjoh marked this pull request as draft June 23, 2023 11:09

banjoh closed this Aug 24, 2023

banjoh force-pushed the em/dump-spec branch from 0fba7bb to 04f69b3 Compare August 24, 2023 14:01

banjoh reopened this Aug 24, 2023

banjoh added 9 commits August 25, 2023 12:30

chore: make specs an internal package

e7ab697

Some minor improvements

20c3a03

Use LoadClusterSpecs in support bundle implementation

e79eb85

Remove change accidentally committed

d04d60f

Use LoadFromCLIArgs in preflight CLI implementation

be50c6c

Update comment

f5ced9e

Fix edge case where the label selector is an empty string

73753e1

Fix failing test

7e9ff94

feat: command to print troubleshoot specs to stdout

8bc624a

* Remove duplicate version.go files

banjoh force-pushed the em/dump-spec branch from 54f7c62 to 8bc624a Compare August 25, 2023 20:43

banjoh added 6 commits August 29, 2023 11:49

Rename dump spec

c68bcdc

Merge remote-tracking branch 'upstream/main' into em/dump-spec

f6addea

feat: preflight --dry-run

96f5945

fix failing validate_specs_test

5bf36b8

Merge branch 'main' into em/dump-spec

15ea289

Remove dump_spec file

f7ac2fe

banjoh changed the title ~~feat: command to dump troubleshoot specs to std out~~ feat: command to print preflight specs to std out Aug 31, 2023

Small change

f23bba3

banjoh changed the title ~~feat: command to print preflight specs to std out~~ feat: Dry run flag to print preflight specs to std out Aug 31, 2023

Remove specs not needed by preflight binary

85ca598

banjoh marked this pull request as ready for review August 31, 2023 18:34

Remove --dry-run from preflight flags used in subcommands

7def396

banjoh requested a review from a team as a code owner August 31, 2023 19:02

xavpaice reviewed Sep 4, 2023

View reviewed changes

banjoh added 4 commits September 5, 2023 19:08

Minor changes from PR comments

2ecb01c

Merge branch 'main' into em/dump-spec

40b985d

Minor godoc update

0053b30

Merge branch 'main' into em/dump-spec

2dde934

xavpaice linked an issue Sep 12, 2023 that may be closed by this pull request

Expose rendered spec after merging via an API and the cli #1024

Closed

2 tasks

xavpaice approved these changes Sep 12, 2023

View reviewed changes

banjoh merged commit b9f4fc4 into replicatedhq:main Sep 12, 2023
22 checks passed

banjoh deleted the em/dump-spec branch September 12, 2023 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Dry run flag to print preflight specs to std out #1240

feat: Dry run flag to print preflight specs to std out #1240

banjoh commented Jun 23, 2023 •

edited

Loading

CLAassistant commented Aug 24, 2023 •

edited

Loading

banjoh commented Aug 31, 2023 •

edited

Loading

xavpaice left a comment

xavpaice Sep 4, 2023

banjoh Sep 5, 2023 •

edited

Loading

banjoh Sep 5, 2023 •

edited

Loading

xavpaice Sep 4, 2023

xavpaice Sep 4, 2023

banjoh Sep 5, 2023 •

edited

Loading

feat: Dry run flag to print preflight specs to std out #1240

feat: Dry run flag to print preflight specs to std out #1240

Conversation

banjoh commented Jun 23, 2023 • edited Loading

Description, Motivation and Context

Checklist

Does this PR introduce a breaking change?

CLAassistant commented Aug 24, 2023 • edited Loading

banjoh commented Aug 31, 2023 • edited Loading

xavpaice left a comment

Choose a reason for hiding this comment

xavpaice Sep 4, 2023

Choose a reason for hiding this comment

banjoh Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

banjoh Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

xavpaice Sep 4, 2023

Choose a reason for hiding this comment

xavpaice Sep 4, 2023

Choose a reason for hiding this comment

banjoh Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

banjoh commented Jun 23, 2023 •

edited

Loading

CLAassistant commented Aug 24, 2023 •

edited

Loading

banjoh commented Aug 31, 2023 •

edited

Loading

banjoh Sep 5, 2023 •

edited

Loading

banjoh Sep 5, 2023 •

edited

Loading

banjoh Sep 5, 2023 •

edited

Loading