-
Notifications
You must be signed in to change notification settings - Fork 687
fix: replace manifest.txt help mechanism with embedded user docs directory #6919
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,4 +55,4 @@ scripts/Brewfile.lock.json | |
| test/fixtures/**/go.sum | ||
| .cursor | ||
| .windsurf | ||
| .claude | ||
| .claude | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,4 @@ _cache | |
| bin | ||
| internal/embedded/_data | ||
| /.bin/ | ||
| internal/helpdocs/cli-commands | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Adding this file to the project will prevent IDE errors between builds | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: does this directory need to go into gitignore
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mmmmh good one, adding - but you get the reasoning behind this file, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we perhaps ignore the directory contents entirely in case an interrupted
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what I meant, gitignore the whole directory and just have this one file.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok! for sure |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| package helpdocs | ||
|
|
||
| import ( | ||
| "io/fs" | ||
| "os" | ||
| "path/filepath" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| "github.com/rs/zerolog" | ||
| ) | ||
|
|
||
| var nonDocChars = regexp.MustCompile(`[^a-zA-Z0-9-]`) | ||
|
|
||
| // CommandHelp indexes embedded or test-supplied CLI command help markdown files. | ||
| type CommandHelp struct { | ||
| files map[string]struct{} | ||
| } | ||
|
|
||
| // NewCommandHelp builds a lookup by walking root on fsys for *.md files. | ||
| func NewCommandHelp(fsys fs.FS, root string) (*CommandHelp, error) { | ||
| docFiles, err := docFilesFromEmbed(fsys, root) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &CommandHelp{files: docFiles}, nil | ||
| } | ||
|
|
||
| // NewCommandHelpFromFiles builds a lookup from an existing filename set. | ||
| func NewCommandHelpFromFiles(files map[string]struct{}) *CommandHelp { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Maybe use https://pkg.go.dev/golang.org/x/tools/godoc/vfs/mapfs and remove this New function.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is deprecated! from your own link :-) |
||
| return &CommandHelp{files: files} | ||
| } | ||
|
|
||
| // HasUserDoc reports whether legacy user-doc help should be shown for command segments. | ||
| // Empty segments -> true (top-level README via legacy help). | ||
| // Non-empty segments -> true only if a matching .md exists (README excluded). | ||
| func (h *CommandHelp) HasUserDoc(segments []string) bool { | ||
| return hasUserDoc(segments, h.files) | ||
| } | ||
|
|
||
| func docFilesFromEmbed(fsys fs.FS, root string) (map[string]struct{}, error) { | ||
| files := make(map[string]struct{}) | ||
| err := fs.WalkDir(fsys, root, func(_ string, d fs.DirEntry, err error) error { | ||
| if err != nil || d.IsDir() || !strings.HasSuffix(d.Name(), ".md") { | ||
| return err | ||
| } | ||
| files[d.Name()] = struct{}{} | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return files, nil | ||
| } | ||
|
|
||
| // helpFileName mirrors src/cli/commands/help/index.ts findHelpFile() join + replace. | ||
| func helpFileName(segments []string) string { | ||
| joined := strings.Join(segments, "-") | ||
| cleaned := nonDocChars.ReplaceAllString(joined, "") | ||
| return cleaned + ".md" | ||
| } | ||
|
|
||
| func hasUserDoc(segments []string, files map[string]struct{}) bool { | ||
| if len(segments) == 0 { | ||
| return true | ||
| } | ||
| if len(files) == 0 { | ||
| // Missing or empty embed at build time: prefer legacy help lookup. | ||
| return true | ||
| } | ||
| for len(segments) > 0 { | ||
| if _, ok := files[helpFileName(segments)]; ok { | ||
| return true | ||
| } | ||
| segments = segments[:len(segments)-1] | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // commandHelpLogger returns a stderr logger when sourceDir is the snyk/cli help tree. | ||
| func commandHelpLogger(helpCLICommandsDir string) *zerolog.Logger { | ||
| repoRoot := filepath.Clean(filepath.Join(helpCLICommandsDir, "..", "..")) | ||
| if _, err := os.Stat(filepath.Join(repoRoot, "cliv2", "go.mod")); err != nil { | ||
| return nil | ||
| } | ||
| return new(zerolog.New(os.Stderr).With().Timestamp().Str("component", "helpdocs").Logger()) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| package helpdocs | ||
|
|
||
| import ( | ||
| "testing" | ||
| "testing/fstest" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func Test_helpFileName(t *testing.T) { | ||
| assert.Equal(t, "container-test.md", helpFileName([]string{"container", "test"})) | ||
| assert.Equal(t, "iac-describe.md", helpFileName([]string{"iac", "describe"})) | ||
| assert.Equal(t, "secrets-test.md", helpFileName([]string{"secrets", "test"})) | ||
| } | ||
|
|
||
| func Test_HasUserDoc(t *testing.T) { | ||
| help := CommandHelpForTest(t) | ||
|
|
||
| tests := map[string]struct { | ||
| segments []string | ||
| want bool | ||
| }{ | ||
| "empty uses readme path": {segments: []string{}, want: true}, | ||
| "test command": {segments: []string{"test"}, want: true}, | ||
| "container test subcommand": {segments: []string{"container", "test"}, want: true}, | ||
| "iac describe subcommand": {segments: []string{"iac", "describe"}, want: true}, | ||
| "unknown command": {segments: []string{"rainmaker"}, want: false}, | ||
| "undocumented secrets test": {segments: []string{"secrets", "test"}, want: false}, | ||
| "redteam setup walks back to parent": {segments: []string{"redteam", "setup"}, want: true}, | ||
| "undocumented agent-scan": {segments: []string{"agent-scan"}, want: false}, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| assert.Equal(t, tc.want, help.HasUserDoc(tc.segments)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func Test_NewCommandHelpFromFS(t *testing.T) { | ||
| expected := fixtureCommandHelp() | ||
|
|
||
| fsMap := fstest.MapFS{ | ||
| "do-not-delete": {Data: []byte("placeholder")}, | ||
| "nested/ignored.md": {Data: []byte("# nested")}, | ||
| } | ||
| for name := range fixtureCommandHelpFiles { | ||
| fsMap[name] = &fstest.MapFile{Data: []byte("# doc")} | ||
| } | ||
|
|
||
| var help *CommandHelp | ||
| var err error | ||
| if help, err = NewCommandHelp(fsMap, "."); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| for _, segments := range [][]string{ | ||
| {"test"}, | ||
| {"container", "test"}, | ||
| {"rainmaker"}, | ||
| } { | ||
| assert.Equal(t, expected.HasUserDoc(segments), help.HasUserDoc(segments), segments) | ||
| } | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: For readability it might be better to integrate these small files (embed.go, and export_test.go) into other relevant files; perhaps command_help.go, and fixture.go, respectively. Other than this, LGTM :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack :-) about embed.go: the Go convention is that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in snyk you can find similar examples https://github.com/snyk/policy-engine/blob/main/rego/embed.go
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. about export_test.go: export_help.go is a kind of standard for test only package helpers. Example: os/export_test.go, fmt/export_test.go, etc.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL, thanks for the explanation! I noticed CommandHelpFromRepo is already used directly in a couple of test files (helprouting_test.go:233 and externally at main_test.go:770), while CommandHelpForTest is only used internally at command_help_test.go:17. Do we need the extra re-export via CommandHelpForTest? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package helpdocs | ||
|
|
||
| import "embed" | ||
|
|
||
| const cliCommandsDir = "cli-commands" | ||
|
|
||
| //go:embed cli-commands | ||
| var cliCommands embed.FS | ||
|
|
||
| var defaultCommandHelp *CommandHelp | ||
|
|
||
| // DefaultCommandHelp returns the compile-time embedded CLI command help lookup. | ||
| func DefaultCommandHelp() *CommandHelp { | ||
| if defaultCommandHelp == nil { | ||
| var err error | ||
| if defaultCommandHelp, err = NewCommandHelp(cliCommands, cliCommandsDir); err != nil { | ||
| panic("helpdocs: index cli-commands" + err.Error()) | ||
| } | ||
| } | ||
| return defaultCommandHelp | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package helpdocs | ||
|
|
||
| import "testing" | ||
|
|
||
| func CommandHelpForTest(t *testing.T) *CommandHelp { | ||
| t.Helper() | ||
| return CommandHelpFromRepo() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package helpdocs | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| ) | ||
|
|
||
| var fixtureCommandHelpFiles = map[string]struct{}{ | ||
| "test.md": {}, | ||
| "container.md": {}, | ||
| "container-test.md": {}, | ||
| "iac.md": {}, | ||
| "iac-describe.md": {}, | ||
| "redteam.md": {}, | ||
| } | ||
|
|
||
| // fixtureCommandHelp returns a minimal doc index for unit tests without compile-time embed. | ||
| func fixtureCommandHelp() *CommandHelp { | ||
| return NewCommandHelpFromFiles(fixtureCommandHelpFiles) | ||
| } | ||
|
|
||
| const repoCLICommandsDir = "../../../help/cli-commands" | ||
|
|
||
| // CommandHelpFromRepo prefers help/cli-commands on disk, else fixtureCommandHelp. | ||
| func CommandHelpFromRepo() *CommandHelp { | ||
| sourceDir, err := filepath.Abs(repoCLICommandsDir) | ||
| if err != nil { | ||
| return fixtureCommandHelp() | ||
| } | ||
| matches, err := filepath.Glob(filepath.Join(sourceDir, "*.md")) | ||
| if err != nil || len(matches) == 0 { | ||
| return fixtureCommandHelp() | ||
| } | ||
| commandHelp, err := NewCommandHelp(os.DirFS(sourceDir), ".") | ||
| if err != nil { | ||
| if logger := commandHelpLogger(sourceDir); logger != nil { | ||
| logger.Warn().Err(err).Str("dir", sourceDir).Msg("failed to load command help from repo") | ||
| } | ||
| return fixtureCommandHelp() | ||
| } | ||
| return commandHelp | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Now that we have
_helpdocs-prepareand_helpdocs-clean. Could it be possible just to generate themanifest.txtwith the filenames, embed that, and then remove it?Will the contents of .md files end up being embedded twice in the binary as
//go:embed cli-commandsdoesn't include strictly just the file names, and I believe they're also embedded by the legacy TS version?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not generating the
manifest.txtdue to the user-docs hook issue; that's the whole purpose of this PR.I do not follow your second comment :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the manifest point, I think we're aligned that a committed manifest.txt is the problem: because of the weekly sync-cli-help-to-user-docs action, the manifest could drift.
What I was wondering is whether a transient manifest (generated in _helpdocs-prepare, embedded for compile, removed in _helpdocs-clean, never committed) would still have that problem. It feels like it would solve the same drift issue, just with a smaller Go-side embed than copying all the .md files. Does that match your thinking, or am I missing something about why even a build-time-only manifest is undesirable?
On the second part, Go's //go:embed cli-commands pulls in the full markdown files (with content), but HasUserDoc only uses their filenames for routing.
Mostly curious whether I'm reasoning about the transient-manifest alternative correctly, or if there's a constraint I'm not seeing. Happy to resolve the thread either way once I've understood your take. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what's the point of generating a
manifest.txtin_helpdocs-prepare. I think that's adding extra complexity:manifest.txtVs what we have here.
embed.FSis basically an iterable construct which does not imply any complexity other than iterating it.I do not think it brings any advantage. If you could name any real, tangible advantage we may be in the same line ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this approach we're embedding the docs content twice in the shipped binary: Go embeds the full .md files but only uses filenames for routing, and the legacy TS binary already embeds the same content for rendering. With a transient manifest we'd only have the filenames on the Go side. The difference isn't big though 😅 .
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be right on one side: yes, double embed. The TS bundle will be there for long time I believe.
Your proposal may work and shrink the Go side; but reopens the initial approach in CLI-1596 and adds more problems to the PR, which is already large.
Planned scope: preparing+cleaning changes, embed, getting a green pipeline (heh), multiple new review threads.
Do you really think it is worth to start changing the whole PR? :)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. Thank you 😀
Approval already given. I believe you can merge this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank youuuu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to add a closing thought here, we will be removing the TS implementation in the future, so the double embed is only temporary.