go generate support for njs/headersmore

[dengsh12]

Proposed changes

Update: apply json config, merged with #112

We plan to provide a tool to update supporting files(analyze_*_directives.go) by go generate. This MR is to provide go generate update for njs/headersmore.

go generate only targets for dynamic modules(or OSS, NPlus) we want to provide long-lasting supports for. To provide temporary support for a module, please refer to #101

Here are the detailed changes:

• Add a shell script to fetch code from github and generate corresponding support files. It would make adding new dynamic modules or updating current existing support files easy.
• Add .gen.go suffix to generated files and ignore them in golangci-lint
• Add go:generate comments for njs/headersmore in analyze.go, now we can update analyze_headersMore_directives.gen.go and analyze_njs_directives.gen.go by go generate

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING document
• If applicable, I have added tests that prove my fix is effective or that my feature works
• If applicable, I have checked that any relevant tests pass after adding my changes
• I have updated any relevant documentation (README.md)

[UnwashedMeme]

Generally should avoid eval unless there is a good reason to.

Suggested change

	generate_command="go run ./cmd/generate --src-path=$tmp --directive-map=$directive_map --match-func=$match_func"
	generate_command="$generate_command > $path"
	eval "$generate_command"
	go run ./cmd/generate "--src-path=$tmp" "--directive-map=$directive_map" "--match-func=$match_func" > $path

should do just fine, no?

If you want to make this a little neater and avoid long lines the pattern I like (requires /bin/bash at the top) is:

Suggested change

	generate_command="go run ./cmd/generate --src-path=$tmp --directive-map=$directive_map --match-func=$match_func"
	generate_command="$generate_command > $path"
	eval "$generate_command"
	genArgs=(
	"--src-path=$tmp"
	# line comments are ok here to describe what you're doing
	"--directive-map=$directive_map"
	"--match-func=$match_func"
	)
	go run ./cmd/generate "${genArgs[@]}" > "$path"

genArgs is a bash array that we then splat (with quoting intact) onto the run command. The really nice thing about this array construct is that it doesn't require the \ to continue lines that makes comments not work and is error prone as you add/remove lines.

[UnwashedMeme]

Another benefit of the bash array is that you can append to it, e.g. genArgs+=(--directive-map=foo)

With that, and the fact that it looks like a lot of the args are being passed through to cmd/generate can you do more of the error handling in there?

E.g. if the the case statement was

    --match_func | -m)
            genArgs+=("--match-func=$2")
            shift
            ;;

and if the cmd/generate doesn't get a match function it raises the error?

[dengsh12]

genArgs is a bash array that we then splat (with quoting intact) onto the run command.

Cool! I didn't know that before. Thanks for telling me this. You are right, the reason I used eval was I don't want long line and bash array seems a perfect solution. I applied it in latest commit.

With that, and the fact that it looks like a lot of the args are being passed through to cmd/generate can you do more of the error handling in there?
E.g. if the the case statement was
--match_func | -m)
genArgs+=("--match-func=$2")
shift
;;
and if the cmd/generate doesn't get a match function it raises the error?

I thought I already did the check here? Just after the arguments reading part

if [ "$url" = "" ]; then
    echo "url can't be empty"
    exit 1
fi

if [ "$path" = "" ]; then
    echo "path can't be empty"
    exit 1
fi

if [ "$directive_map" = "" ]; then
    echo "directive-map can't be empty"
    exit 1
fi

if [ "$match_func" = "" ]; then
    echo "match-func can't be empty"
    exit 1
fi

[UnwashedMeme]

yeah the array-splat is one of my favorite bash tricks, e.g. look at the use of "opts" separately in most functions in https://gitlab.com/f5/nginx/nginxazurelb/azure-infrastructure/-/snippets/2371626

[UnwashedMeme]

I gave some ideas for parsing the args separately below, but maybe public_git should just do the clone and not call generate.
E.g. what about having this line be:

Suggested change

	//go:generate sh ./scripts/generate/public_git.sh --url https://github.com/openresty/headers-more-nginx-module.git -p ./analyze_headersMore_directives.gen.go -m HeadersMoreDirectivesMatchFn -d moduleHeadersMoreDirectives
	//go:generate sh go run ./cmd/generate --src-path=$(./scripts/generate/public_git.sh --url https://github.com/openresty/headers-more-nginx-module.git) -p ./analyze_headersMore_directives.gen.go -m HeadersMoreDirectivesMatchFn -d moduleHeadersMoreDirectives

public_git does the clone and prints out the the directory that becomes the src-path. Now public-git doesn't need to know anything about cmd/generate or its args.

[dengsh12]

Oh there are some considerations for OSS and Lua in next MR. In next MR we will add OSS and Lua into go generate, and the arguments(-filter and -override) for ./cmd/generate will be long. Here the command line for lua:

go run ./cmd/generate --src-path=/var/folders/pz/_91zbp7d7fb4mpclbtmzw4rr0000gp/T/tmp.uc45dimtKO --directive-map=moduleLuaDirectives --match-func=LuaDirectivesMatchFn -filter=lua_fake_shm  -override="content_by_lua_block:ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake1" -override="rewrite_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake1" -override="ssl_certificate_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxConfTake1" -override="ssl_session_fetch_by_lua_block:ngxHTTPMainConf | ngxConfTake1" -override="ssl_session_store_by_lua_block:ngxHTTPMainConf | ngxConfTake1" -override="set_by_lua_block:ngxHTTPSrvConf | ngxHTTPSifConf | ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake2" -override="ssl_client_hello_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxConfTake1" -override="body_filter_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake1" -override="init_by_lua_block:ngxHTTPMainConf | ngxConfTake1" -override="init_worker_by_lua_block:ngxHTTPMainConf | ngxConfTake1" -override="header_filter_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake1" -override="log_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake1" -override="access_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxHTTPLocConf | ngxHTTPLifConf | ngxConfTake1" -override="balancer_by_lua_block:ngxHTTPUpsConf | ngxConfTake1" -override="server_rewrite_by_lua_block:ngxHTTPMainConf | ngxHTTPSrvConf | ngxConfTake1" -override="exit_worker_by_lua_block:ngxHTTPMainConf | ngxConfTake1" > ./analyze_lua_directives.go

We don't want this as a long line of go:generate comment. We plan to put the -filter and -override arguments in a separate shell script, or in a file and let public-git.sh to read that file in next MR. So we put the part of invoking ./cmd/generate here. And the purpose of public_git.sh is only called by go generate to update .gen.go files. I just added some comments to illustrate this.

[UnwashedMeme]

I haven't worked in this repo yet so I'm coming in a bit blind; trying to assemble some pieces here:

./cmd/generate may be called by some users just want to generate a temporary support for a third-party module, so I provide a default value just for their convenience.

there are some considerations for OSS and Lua in next MR. In next MR we will add OSS and Lua into go generate, and the arguments(-filter and -override) for ./cmd/generate will be long. Here the command line for lua: ...

plan to put the -filter and -override arguments in a separate shell script

Your suggesting a user might do the git clone manually and call cmd/generate once and then build directly (never adding go:gen statements). That on that third party library they wouldn't need to do a directive-map/match-func, it's just that the lua and headersmore are super weird and need it. We're trying to include headers/lua as standard components to this repo.

Idea 1 - lots of shell

• public-git.sh should only be responsible for doing the clone into temp
• makea generate/lua.sh and generate/headersmore.sh that call public-git and then cmd/generate calls those, encode all the details

idea 2 - more go

• public-git should still be only responsible for doing the clone
• encode more of these options into cmd/generate and make a --module or --type or --preset flag to get there.

(i'll try and come back here with more detail later, but have to go now)

[dengsh12]

I haven't worked in this repo yet so I'm coming in a bit blind; trying to assemble some pieces here:

./cmd/generate may be called by some users just want to generate a temporary support for a third-party module, so I provide a default value just for their convenience.

there are some considerations for OSS and Lua in next MR. In next MR we will add OSS and Lua into go generate, and the arguments(-filter and -override) for ./cmd/generate will be long. Here the command line for lua: ...

plan to put the -filter and -override arguments in a separate shell script

Your suggesting a user might do the git clone manually and call cmd/generate once and then build directly (never adding go:gen statements). That on that third party library they wouldn't need to do a directive-map/match-func, it's just that the lua and headersmore are super weird and need it. We're trying to include headers/lua as standard components to this repo.

Idea 1 - lots of shell

• public-git.sh should only be responsible for doing the clone into temp
• makea generate/lua.sh and generate/headersmore.sh that call public-git and then cmd/generate calls those, encode all the details

idea 2 - more go

• public-git should still be only responsible for doing the clone
• encode more of these options into cmd/generate and make a --module or --type or --preset flag to get there.

(i'll try and come back here with more detail later, but have to go now)

Oh I should've provided more context here. go generate is provided to update the .gen.go files in crossplane, or add a new dynamic module to crossplane. In the future all analyze_*_directives.go in crossplane will turn into .gen.go files. To update crossplane we need a MR each time. Once a //go:generate statement added to crossplane, the module is considered officially supported by crossplane.

cmd/generate targets for users with a third-party module, say MD. MD is not officially supported by crossplane(it may be just a module invented by the user, doesn't have a github link). The user may want to use crossplane to parse MD, but he doesn't want to open a MR for it. In this case he can call cmd/generate directly. I just added this into MR description.

That on that third party library they wouldn't need to do a directive-map/match-func, it's just that the lua and headersmore are super weird and need it.

Nah, I provided a default directive-map/match-func for third party library. I just wanted some convenience when they invoke cmd/generate. It seems the default parameter not that make sense and cause confusing. I will change them to required.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.09%. Comparing base (82990a0) to head (3d63a28).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
+ Coverage   78.82%   79.09%   +0.26%     
==========================================
  Files          26       26              
  Lines        1124     1124              
==========================================
+ Hits          886      889       +3     
+ Misses        182      179       -3     
  Partials       56       56              

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