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

Makefile: add check-migrations task to check for common problems #3051

Conversation

markusboehme
Copy link
Member

Issue number: n/a

Description of changes:

Add a check-migrations task that runs as part of check. It attempts to check for common problems with migrations. It might be nice to have it run during build tests. While it does not actually run migrations or check their logic, it can spot the kind of inconsistencies that are easily lost in reviews.

Testing done:

I caused a typo in Release.toml and:

$ cargo make check-migrations 
[cargo-make] INFO - cargo make 0.36.7
[cargo-make] INFO - Build File: Makefile.toml
[cargo-make] INFO - Task: check-migrations
[cargo-make] INFO - Profile: development
[cargo-make] INFO - Running Task: check-migrations
Found 2 problem(s) with data store migrations:
    - Migration 'qubelet-config-settings' does not exist
    - Migration 'kubelet-config-settings' is missing a declaration in Release.toml
[cargo-make] ERROR - Error while executing command, exit code: 1
[cargo-make] WARN - Build Failed.

Other scenarios tested include: a release without any migrations; various ways to list migrations in Release.toml (who needs a TOML parser with Perl regular expressions?)

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Add a `check-migrations` task that runs as part of `check`. It attempts
to check for common problems with migrations. It might be nice to have
it run during build tests. While it does not actually run migrations or
check their logic, it can spot the kind of inconsistencies that are
easily lost in reviews.

Signed-off-by: Markus Boehme <markubo@amazon.com>
@markusboehme
Copy link
Member Author

This PR would only introduce the new cargo make task, not yet run it on PRs. If this is something we want: I was wondering about how to best do this. The simplest way would be to add another cargo make check- invocation to the build.yml workflow, but since none of the checks are variant-specific it might be wasteful to have rerun all builds for a change fixing up migrations meta data. Then again it is what we do for linting shell scripts, and it's a rare enough occurrence to not bother with it if it's too much work to split out.

Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

This PR would only introduce the new cargo make task, not yet run it on PRs. If this is something we want: I was wondering about how to best do this. The simplest way would be to add another cargo make check- invocation to the build.yml workflow, but since none of the checks are variant-specific it might be wasteful to have rerun all builds for a change fixing up migrations meta data. Then again it is what we do for linting shell scripts, and it's a rare enough occurrence to not bother with it if it's too much work to split out.

This is great! I really like this.

As far as checks, I think we could add another workflow file that has the condition that it only gets triggered on changes to either Release.toml or anything under sources/api/migration/**, similar to how we only trigger on Go related things in the golangci-lint check.

I think the automatic check would be ideal, but that could also be a follow on to this. Just having a manual way to perform some validation is a huge improvement over what we have now (nothing).

Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

Nice!

@markusboehme
Copy link
Member Author

This PR would only introduce the new cargo make task, not yet run it on PRs. If this is something we want: I was wondering about how to best do this. The simplest way would be to add another cargo make check- invocation to the build.yml workflow, but since none of the checks are variant-specific it might be wasteful to have rerun all builds for a change fixing up migrations meta data. Then again it is what we do for linting shell scripts, and it's a rare enough occurrence to not bother with it if it's too much work to split out.

As far as checks, I think we could add another workflow file that has the condition that it only gets triggered on changes to either Release.toml or anything under sources/api/migration/**, similar to how we only trigger on Go related things in the golangci-lint check.

That looks way better than tacking it onto the build step. I'll try my hand at an independent workflow taking inspiration from the Go linting one (hoping to be able to test this just the same in my fork first). Thanks for the pointer!

@markusboehme markusboehme merged commit 3be8d26 into bottlerocket-os:develop May 8, 2023
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants