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

Normalization use pinned version #1209

Merged
merged 1 commit into from
Dec 5, 2020
Merged

Normalization use pinned version #1209

merged 1 commit into from
Dec 5, 2020

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Dec 5, 2020 •
edited

What

  • I'm hating this PR: Version Normalization as if it were part of core #1208. It is spiraling out of control. The number of dependency related hacks is too big. We're going to mess it up. The straw that broke the camel's back was needing to depend on composeBuild (example). That is really ugly.
  • Proposing we do what @sherifnada said in the first place and just pin the version. The single, but big, downside of this is when we change normalization we need to remember to bump the version AND to change the version in DefaultNormalizationRunner.

michel-tricot
michel-tricot approved these changes Dec 5, 2020
View reviewed changes
Copy link
Contributor

@michel-tricot michel-tricot left a comment

Choose a reason for hiding this comment

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

Sometime the simplest is the best...

@cgardens cgardens merged commit 917f3d4 into master Dec 5, 2020
@cgardens cgardens deleted the cgardens/version_normalization2 branch December 5, 2020 00:42
sherifnada
sherifnada approved these changes Dec 5, 2020
View reviewed changes
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

I'm definitely worried about forgetting this on a release one day. I don't know enough about DI, but I wonder if there's a flavor of it that:
a. doesn't require passing airbyteVersion throughout the callchain into NormalizationRunner
b. can be easily mocked in tests

@cgardens cgardens mentioned this pull request Dec 5, 2020
Closed
@cgardens
Copy link
Contributor Author

cgardens commented Dec 7, 2020

I'm definitely worried about forgetting this on a release one day.

@sherifnada I'm not sure I understand this concern. Are you just worried that when we update the container we'll forget to bump the version?

@sherifnada
Copy link
Contributor

Yup

@cgardens
Copy link
Contributor Author

cgardens commented Dec 7, 2020

Fair. Yeah, I don't have a good answer right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michel-tricot michel-tricot michel-tricot approved these changes

@sherifnada sherifnada sherifnada approved these changes

@jrhizor jrhizor Awaiting requested review from jrhizor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cgardens @sherifnada @michel-tricot