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.

Sign up for GitHub

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

Jump to bottom

Make checksum comparison try matching with lf and crlf line endings #2101

Merged
merged 2 commits into from Aug 3, 2021
Merged

Make checksum comparison try matching with lf and crlf line endings #2101

merged 2 commits into from Aug 3, 2021

Conversation

tomhoule
Copy link
Contributor

@tomhoule tomhoule commented Jul 19, 2021 •
edited

@tomhoule tomhoule added this to the 2.28.0 milestone Jul 19, 2021
@tomhoule tomhoule changed the title Encapsulate checksum handling into a module in MigrationConnector Make checksum comparison try matching with lf and crlf line endings Jul 19, 2021
tomhoule
tomhoule commented Jul 19, 2021
View reviewed changes
migration-engine/connectors/migration-connector/src/migrations_directory.rs
buf.extend_from_slice(bytes.as_ref());

Ok(())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was dead code :(

@tomhoule tomhoule force-pushed the migration-engine/checksum-newlines branch 2 times, most recently from bde1f34 to 3d8b436 Compare July 19, 2021 12:37
@tomhoule tomhoule marked this pull request as ready for review July 19, 2021 12:46
@tomhoule tomhoule added the tech/engines/migration engine Issue in the Migration Engine label Jul 19, 2021
@tomhoule tomhoule force-pushed the migration-engine/checksum-newlines branch from 3d8b436 to ebc05be Compare July 23, 2021 06:08
@Jolg42 Jolg42 added process/candidate team/schema Issue for team Schema. labels Jul 26, 2021
@Jolg42
Copy link
Member

Jolg42 commented Jul 26, 2021

From Tom: will need a careful review but I think it's complete.

@Jolg42 Jolg42 requested a review from do4gr July 26, 2021 15:28
do4gr
do4gr approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@do4gr do4gr left a comment

Choose a reason for hiding this comment

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

LGTM

@do4gr do4gr requested a review from pimeys August 2, 2021 09:34
pimeys
pimeys approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@pimeys pimeys left a comment

Choose a reason for hiding this comment

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

for loops kind of caused a bit of a friction when reading this, but I can approve this!

@Jolg42 Jolg42 modified the milestones: 2.28.0, 2.29.0 Aug 2, 2021
@do4gr do4gr merged commit 10a1bec into master Aug 3, 2021
@do4gr do4gr deleted the migration-engine/checksum-newlines branch August 3, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pimeys pimeys pimeys approved these changes

@do4gr do4gr do4gr approved these changes

Assignees
No one assigned
Labels
team/schema Issue for team Schema. tech/engines/migration engine Issue in the Migration Engine
Projects
None yet
Milestone
2.29.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tomhoule @Jolg42 @pimeys @do4gr