Add new linter for PK type

[morgo]

This introduces a new linter for the PK type. It checks all the PK parts, and validates that the type exists in allow-pk-type. The motivation for this linter, is that some tools (including a schema change tool we are building) require memory comparable primary keys to safely operate.

The default list of allow-pk-type includes the types that we expect tables will feature (i.e. roughly all types excluding non-memory comparable types) (see discussion; there is no default). It can be modified by other users as required to suit their purposes. The linter itself is set to SeverityIgnore by default.

Edit: The motivation to add a new linter vs. extend the exiting linter to require PK, is that it's likely a user might want one to be an error, and another a warning. We will likely introduce this linter in our infrastructure this way.

Fixes #204

[evanelias]

Thank you so much for the pull request! I'll aim to write some review comments later today.

btw, CI is failing due to a pre-existing bug in the linter package's test suite setup, not your fault. If a linter rule defaults to SeverityIgnore and also uses RelatedListOption, apparently the related allow-list doesn't get set up properly for the test, which later causes a panic when the allow-list isn't present in the expected place in the configuration map. By coincidence, all of the other linter rules with allow-lists happen to default to being enabled, so this condition never came up previously.

I'll work on getting a patch for that test suite bug into the main branch soon, so that CI can run after you rebase against it.

[evanelias]

Just committed a1a5a6d to main, which fixes the test panic. If you can please rebase your PR against that, tests should run properly now. Sorry about that -- thanks for your patience!

The integration test suite will automatically set your linter rule to warning level, and run it against all of the definitions in internal/linter/testdata/validcfg/*.sql. These .sql files have embedded comments which indicate which linter annotations are expected for any given line; the test logic parses those comments and confirms the expected annotations are emitted, as well as no additional unexpected annotations.

Currently all the table definitions in that directory have an int PK or no PK, so tests will still pass as-is, but it won't actually provide coverage for triggering your new linter check. In order to cover that in the test, simply add a couple things to that testdata directory:

• Add a line to internal/linter/testdata/validcfg/.skeema to configure allow-pk-type to some value which includes int at minimum but excludes some other types like maybe varchar. (I know your proposed default value already does that, but will discuss that more in review comments later today -- in any case let's just be explicit in the test configuration)
• Create a new file internal/linter/testdata/validcfg/badpk.sql which creates a table using a PK with a data type that isn't on the allow-pk-type list. Add an annotation comment on the line corresponding to the problematic column; see autoinc.sql or multibad.sql for example format. If desired, you can put multiple tables in this file, if you want to test single-column vs compound PKs.

Thanks again! I'll submit a PR review with some other minor notes later today.

[evanelias]

Thanks again for the PR, overall this is excellent! Just a few nits, plus the test coverage mentioned in previous comment, and then this should be good to merge

[morgo]

Sorry I don't really understand the test-runner. I did run it locally, and I'm getting the same error against main as I am in this branch, so I assume that's a good thing.

[evanelias]

Thanks, almost there!

The test output is really noisy, apologies for not describing this better -- don't worry about all the [ERROR] log lines, basically that's just from it running various code paths that intentionally trigger errors.

When the tests report failure, I just search the output for "FAIL:" to see which subtest failed and then scroll up to see the specific test error lines. It's definitely not the best or easiest, but hopefully it's usable. In this case the relevant log lines start at https://github.com/skeema/skeema/actions/runs/4417888202/jobs/7744904084#step:7:12092 and are just about extra unexpected linter annotations, and one annotation on the wrong line.

To reproduce locally on a machine with Docker, in this example just testing against MySQL 8 and only running this specific failing subtest:

cd internal/linter
SKEEMA_TEST_IMAGES=mysql:8.0 go test -v -run Integration/TestCheckSchema

Thanks again for your patience with the current bumpy PR process. I definitely want to make it easier and smoother to do linter PRs (and Skeema PRs in general) in the near future, just haven't been able to devote much time to that yet.

[morgo]

Thanks again for your patience with the current bumpy PR process. I definitely want to make it easier and smoother to do linter PRs (and Skeema PRs in general) in the near future, just haven't been able to devote much time to that yet.

No problem, thanks for the review & fast turn around!

[coveralls]

Coverage: 93.263% (+0.03%) from 93.23% when pulling ec51c27 on morgo:lint-pk-type into 93ad4c4 on skeema:main.

[evanelias]

Looks perfect! Merging momentarily.