Skip to content
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

Add help system to Makefile + FDM command #25028

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Add help system to Makefile + FDM command #25028

wants to merge 27 commits into from

Conversation

sgress454
Copy link
Contributor

This PR adds a help system to the current Fleet Makefile to make it easier to add and maintain documentation for the various commands it contains. For any command foo in the Makefile, various targets can be added to provide help:

  • .help-short--foo: print a terse description of the command, e.g. "build the binaries". This will be used both in the main help output which lists all available commands, and in the individual command help.
  • .help-long--foo: print a longer description of the command, e.g. "Builds the specified binaries (defaults to building fleet and fleetctl)". Can be multi-line.
  • .help-options--foo: print options for the command as pairs of lines, with the first line being the option name (i.e. the makefile var name) and the second being a description of the option. For the "build" command, we output:
FLEET
Build the fleet binary only
FLEETCTL
Build the fleetctl binary only
GO_BUILD_RACE_ENABLED
Turn on data race detection when building
EXTRA_FLEETCTL_LDFLAGS="--flag1 --flag2..."
Flags to provide to the Go linker when building fleetctl

The .help-long and .help-options variants are mainly for use with the new fdm command (see below), but can also be utilized in make invocations using the SPECIFIC_CMD var, e.g. make help SPECIFIC_CMD=build.

The fdm command

This PR also introduces a new fdm utility that wraps the makefile and translates arguments to the various targets into more idiomatic CLI arguments; for example

make build GO_BUILD_RACE_ENABLED=true FLEET=true

can be called with

fdm build --go-build-race-enabled=true --fleet

fdm help will list all commands that have at least .help-short- targets. I'd advise doing this for developer-facing commands like build. In the future we can provide a similar mechanism for adding help to more esoteric commands like nudge-app-tar-gz and make the full command list available via something like fdm help --all.

fdm help <command> will output full help for a command, e.g. fdm help build gives you:

NAME:
  build - Build binaries

DESCRIPTION:
  Builds the specified binaries (defaults to building fleet and fleetctl)

OPTIONS:
  --fleet                                        Build the fleet binary only
  --fleetctl                                     Build the fleetctl binary only
  --go-build-race-enabled                        Turn on data race detection when building
  --extra-fleetctl-ldflags="--flag1 --flag2..."  Flags to provide to the Go linker when building fleetctl

The fdm command can be built with make fdm, which also symlinks it into /usr/local/bin for ease of use.

@sgress454 sgress454 requested a review from a team as a code owner December 27, 2024 19:42
@sgress454 sgress454 requested a review from iansltx December 27, 2024 19:43
Makefile Outdated
Comment on lines 78 to 83
BINS_TO_BUILD = fleet fleetctl
ifdef FLEET
BINS_TO_BUILD = fleet
else ifdef FLEETCTL
BINS_TO_BUILD = fleetctl
endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow building either fleet or fleetctl or both, all through the build command (so you can do fdm build --fleet or fdm build --fleetctl, or just fdm build to do both)

Copy link

codecov bot commented Dec 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.58%. Comparing base (51fecdd) to head (120dedf).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25028      +/-   ##
==========================================
- Coverage   63.59%   63.58%   -0.01%     
==========================================
  Files        1619     1619              
  Lines      154976   154995      +19     
  Branches     4038     4038              
==========================================
+ Hits        98552    98559       +7     
- Misses      48654    48662       +8     
- Partials     7770     7774       +4     
Flag Coverage Δ
backend 64.43% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sgress454 sgress454 changed the title Fdm Add help system to Makefile + FDM command Dec 28, 2024
Copy link
Member

@iansltx iansltx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pulled this down and there's no obvious way to get the longer help text while running make. Probably just missing something but e.g. "make help build" shows help and then builds, rather than help consuming all arguments to show the right thing.

Guessing make fdm fixes this but it's not listed in the help, and the symlink fails:

~/code/fleet fdm $ make fdm 
go build -o build/fdm ./tools/fdm
ln -sf "$(pwd)/build/fdm" /usr/local/bin/fdm	
ln: /usr/local/bin/fdm: Permission denied
make: *** [fdm] Error 1

Makefile Outdated
.help-long--build:
@echo "Builds the specified binaries (defaults to building fleet and fleetctl)"
.help-options--build:
@echo "FLEET"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work all-caps? I've only ever used e.g. make fleetctl.

Makefile Outdated
test: lint test-go test-js

.help-short--generate:
@echo "Generate and bundle required all code"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we're moving this around, probably better to revise this description to be more accurate, as we have a bunch of generate commands and this only builds bindata on the go side (vs. commands that update mocks, schema, etc.)

Makefile Outdated
generate-dev: .prefix
NODE_ENV=development yarn run webpack --progress
go run github.com/kevinburke/go-bindata/go-bindata -debug -pkg=bindata -tags full \
-o=server/bindata/generated.go \
frontend/templates/ assets/... server/mail/templates
NODE_ENV=development yarn run webpack --progress --watch

.help-short--generate-mock:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're actively working on this PR, mind making the tweaks mentioned in this Slack thread on command naming?

@sgress454
Copy link
Contributor Author

Ha, adding help for the fdm command would probably be a good idea 😂. As far as the symlink, it worked for me but it's possible it may require sudo for some users; I can add that. You can try ./build/fdm in the meantime.

Getting long help with make is a little cumbersome -- you can do it with SPECIFIC_CMD= but it's really more intended to be used with fdm.

@sgress454
Copy link
Contributor Author

@iansltx circled back to this with some updates. I did a bit of a refactor to allow positional arguments in commands, so you can now do e.g.

fdm build fleet

or

fdm lint go

Positional arguments are added to the make call as ARG1, ARG2, etc., so you can technically do:

make lint ARG1=go

but obviously the idea is to have fdm be a more accessible way to do it.

I also simplified the make version of help a bit, so you can get individual command help with e.g.

make help CMD=build

When it comes to actually building binaries, make fleet or make fleetctl makes perfect sense -- it is make after all! It's really about making a central place for all dev commands, many of which won't simply be about building binaries. And making help more accessible and more encouraging to write.

image

make generate-doc
make doc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per discussion here, this PR changes the Makefile targets:

  • generate-doc -> doc
  • generate-mock -> mock
  • dump-test-schema -> test-schema

The old targets are still there as aliases.

Comment on lines -158 to -171
DEFAULT_PKG_TO_TEST := ./cmd/... ./ee/... ./orbit/pkg/... ./orbit/cmd/orbit ./pkg/... ./server/... ./tools/...
ifeq ($(CI_TEST_PKG), main)
CI_PKG_TO_TEST=$(shell go list ${DEFAULT_PKG_TO_TEST} | grep -v "server/datastore/mysql" | grep -v "cmd/fleetctl" | grep -v "server/vulnerabilities" | sed -e 's|github.com/fleetdm/fleet/v4/||g')
else ifeq ($(CI_TEST_PKG), integration)
CI_PKG_TO_TEST="server/service"
else ifeq ($(CI_TEST_PKG), mysql)
CI_PKG_TO_TEST="server/datastore/mysql/..."
else ifeq ($(CI_TEST_PKG), fleetctl)
CI_PKG_TO_TEST="cmd/fleetctl/..."
else ifeq ($(CI_TEST_PKG), vuln)
CI_PKG_TO_TEST="server/vulnerabilities/..."
else
CI_PKG_TO_TEST=$(DEFAULT_PKG_TO_TEST)
endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this down next to the test-go target.

Comment on lines -173 to -174
ci-pkg-list:
@echo $(CI_PKG_TO_TEST)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dantecatalfamo I think this was just for debugging?

Comment on lines +235 to +248
.help-short--test-go:
@echo "Run Go tests for CI"
.help-long--test-go:
@echo "Run one or more bundle of Go tests. These are bundled together to try and make CI testing more parallelizable (and thus faster)."
.help-options--test-go:
@echo "CI_TEST_PKG=[test package]"
@echo "The test package bundle to run. If not specified, all Go tests will run."
.help-extra--test-go:
@echo "AVAILABLE TEST BUNDLES:"
@echo " integration"
@echo " mysql"
@echo " fleetctl"
@echo " vuln"
@echo " main (all tests not included in other bundles)"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

😉

Comment on lines +10 to +20
# Print separators between help sections.
.help-sep-1:
@printf "\036"
.help-sep-2:
@printf "\036"
.help-sep-3:
@printf "\036"
.help-sep-4:
@printf "\036"
.help-sep-5:
@printf "\036"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get make to repeat the same target no matter what I tried, so I just added multiple targets that do the same thing. See makehelp.sh for where they're used.

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

Successfully merging this pull request may close these issues.

2 participants