-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: migration command #5506
feat: migration command #5506
Conversation
The structures are altered to use pointer on builtin types. The field version is added to detect already migrated file.
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.
Nice!! I've yet to review the individual migrations but everything else looks great! I don't think there's much to add for the rest of the code given all the tests but I saw you did a great job on documenting all permutations of enabled and disabled linters that I plan to give an extra read.
🔥
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.
Awesome work.
Left some comments.
Will take a look again later today, still need to review few files.
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.
Awesome, LGTM
Did you think about using the new omitzero tag from go 1.24? We used that in https://github.com/majewsky/gg . See sapcc/keppel#489 as an example where we used it. |
Yes, but:
|
pkg/commands/internal/migrate/testdata/linters-settings_staticcheck.golden.yml
Outdated
Show resolved
Hide resolved
pkg/commands/internal/migrate/testdata/linters-settings_depguard.yml
Outdated
Show resolved
Hide resolved
c0142fe
to
d35e0d4
Compare
I want to explain my journey on this topic because it was not an easy one.
My first thought was to try to keep comments by using several YAML parsing libs, but it was very complex, only for YAML (we should handle YAML, TOML, JSON), and at the end it was not working mainly due to the migration path of some elements.
So I gave up on migrating the comments.
I tried to use the v2 configuration structures directly, but it's not possible to serialize the configuration as expected (even with additional struct tags), mainly because of default non-zero values (e.g. boolean
true
by default, explicit0
orfalse
, ...). So it didn't work.My next try was the working solution: using a clone of the v2 configuration that uses pointer for builtin types and struct tags.
I created a "cloner" which clones the v2 configuration structures (only) and modifies them by changing types and adds struct tags.
So the "cloned" configuration structures can be recreated if needed (
make clone_config
).The solution is stable, maintainable, and tested, but I'm sad that I couldn't find a solution to keep the comments.
The migration file format is based on the extension of the configuration file.
The format can be overridden by using
--format
flag. (ex:golangci-lint migrate --format yml
)Before the migration, the previous configuration file is copied and saved to a file named
<config_file_name>.bck.<config_file_extention>
.By default, before the migration process the configuration file is validated on the JSONSchema of the configuration v1.
If you want to skip this validation, you can use
--skip-validation
. (ex:golangci-lint migrate --skip-validation
)Some notable elements
The migration command also enforced some new default values:
run.timeout
: the existing value is ignored, because, in v2, by default, there is no timeout.issues.show-stats
: the existing value is ignored, in v2, by default, the stats are enabled.run.concurrency
: if the existing value was0
, as it's the new default, the key is removed.run.relative-path-mode
: if the existing value wascfg
, as it's the new default, the key is removed.issues.exclude-generated
has a new default value (v1lax
, v2strict
), then this field will be added during the migration to keep the previous behavior.issues.exclude-dirs-use-default
has been removed, so this is converted tolinters.exclusions.paths
and, if needed,formatters.exclusions.paths
.Deprecated options from the v1 or unknown fields are not migrated.
Other fields, which are explicitly defined in the configuration file, are migrated even if the value is the same as the default value.
A migration guide will be added to the documentation #5439
I created more than 150 test configuration files, and added dedicated tests on complex migration paths.
The command documentation will be added to the migration guide.