nginxinc / nginx-go-crossplane

A library for working with NGINX configs in Go
Apache License 2.0
46 stars 12 forks source link

Provides more `MatchFunc` and code movement for it #100

Closed dengsh12 closed 1 month ago

dengsh12 commented 2 months ago

Proposed changes

In #97 , we provided a new field DirectiveSources in ParseOptions. It is an array of MatchFunc to specify the OSS/N+ version, and dynamic modules to include for validation.

In this PR, we

Checklist

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

ornj commented 2 months ago

I think all the code with the directive maps should have a analyze_ prefix instead of module_ or anything else. This way they will appear in editors near the main analyze.go file.

dakshinai commented 1 month ago

@ornj @dengsh12 What will be the process to add a new N+ version, say R32, add it to NPlusLatest?

I do see see an overlapping list for R30 and R31, I believe specific version lists should include everything up until the version to be able to perform version specific validation? So for R32, create one if needed?

dengsh12 commented 1 month ago

@ornj @dengsh12 What will be the process to add a new N+ version, say R32, add it to NPlusLatest?

@dakshinai It will be in later merge. We will provide a generator. If a new N+ version comes just run go generate ./.... It will fetch from some sources and create new support files.

I do see see an overlapping list for R30 and R31, I believe specific version lists should include everything up until the version to be able to perform version specific validation?

The support file for a specific version only includes the directive definitions in the source code of that version. Say the version is R31, analyze_nplus_R31_directives.go only includes the directive definitions in Nplus R31 source code(so if a directive was removed before R31 it won't appear). The field DirectiveSources provides high flexibility, you can pass MatchFunc for both R30 and R31 to include both of them for validation. I think this way is more flexible and clear?

So for R32, create one if needed?

Currently latest version is R32 so just use analyze_nplus_latest_directives.go can cover. This file will update, say R33 will release tomorrow and this file will update to R33 tomorrow, and a analyze_nplus_R32_directives.go will be generated(since it is not latest). I may caution provide both R32 and analyze_nplus_latest_directives.go will make it confused?(I assume user won't need a no-update R32 while it is the latest version)

Feel free to lmk if there're further concerns!