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

feat(migrations): implement info command #7145

Merged
merged 15 commits into from
Mar 3, 2021

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Mar 3, 2021

Description

This PR implements the migrations info command. The command does two things: prints the current migration version, and prints a table with information about the different available migration versions.

As part of implementing info, this PR also adds a new column (error_reason) to the migrations metadata stream and table, and contains some minor refactors such as renaming Migration to MigrationFile, renaming VersionInfo to MigrationVersionInfo and moving it into its own file, and cleaning up some test code. The refactors are in the first few commits and can be reviewed separately if that's easier.

Testing done

Unit + integration testing.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@vcrfxia vcrfxia requested a review from a team as a code owner March 3, 2021 09:29
// verify FOO and BAR were registered
describeSource("FOO");
describeSource("BAR");

// verify current
final List<StreamedRow> current = makeKsqlQuery("SELECT * FROM migration_schema_versions WHERE VERSION_KEY='CURRENT';");
assertThatEventually(() -> current.size(), is(2));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These old assertThatEventually calls didn't actually do anything since current wasn't being refreshed (i.e., makeKsqlQuery() was only being called once, outside the assertThatEventually). I've gone through and cleaned up the usage of assertThatEventually in this test file.

.map(LoggingEvent::getRenderedMessage)
.collect(Collectors.toList());
assertThat(logMessages, hasItem(containsString("Current migration version: 2")));
assertThat(logMessages, hasItem(matchesRegex(
Copy link
Contributor Author

@vcrfxia vcrfxia Mar 3, 2021

Choose a reason for hiding this comment

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

This integration test uses a regex matcher since (1) the timestamp depends on when the test is run, and (2) the timezone used is local.

public final class MigrationVersionInfo {

private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss.SSS z";
private static final String EMPTY_MIGRATION_TIMESTAMP = "N/A";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also use this value for running migrations instead of an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strongly. I think we can leave it as is. In all likelihood, the only way users will view the contents of the metadata stream and table will be through migrations info, rather than inspecting the stream/table directly.

@vcrfxia vcrfxia merged commit 956e799 into confluentinc:master Mar 3, 2021
@vcrfxia vcrfxia deleted the migrations-info branch March 3, 2021 23:07
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

2 participants