Skip to content

Emit STATE values to stdout instead of the whole state message#142

Merged
AlexanderMann merged 1 commit intodatamill-co:masterfrom
airhorns:no-state-message-wrapper
Sep 9, 2019
Merged

Emit STATE values to stdout instead of the whole state message#142
AlexanderMann merged 1 commit intodatamill-co:masterfrom
airhorns:no-state-message-wrapper

Conversation

@airhorns
Copy link
Copy Markdown
Contributor

@airhorns airhorns commented Aug 23, 2019

This fixes #141 and should improve https://gitlab.com/meltano/meltano/issues/884.

Before this change, target-postgres worked slightly differently than the other taps out there and wrote incoming STATE messages to stdout as the full content of the mesage, including the {"value": ...} wrapper around the opaque STATE JSON. Other taps, including the official singer-target-template, don't write out that value wrapper, and just write the JSON blob contained in it.

This brings target-postgres inline with the other targets and the defacto rule.

This fixes datamill-co#141 and should improve https://gitlab.com/meltano/meltano/issues/884.

Before this change, target-postgres worked slightly differently than the other taps out there and wrote incoming STATE messages to stdout as the full content of the mesage, including the `{"value": ...}` wrapper around the opaque STATE JSON. Other taps, including the official s`inger-target-template`, don't write out that `value` wrapper, and just write the JSON blob contained in it.

This brings target-postgres inline with the other targets and the defacto rule.
@airhorns
Copy link
Copy Markdown
Contributor Author

Also of note is that this is a breaking change and I think merits a new version. Sorry about that!

@AlexanderMann
Copy link
Copy Markdown
Collaborator

@awm33 curious as to your thoughts on whether we should bump the major, minor, or patch for this change?

@aroder
Copy link
Copy Markdown
Contributor

aroder commented Aug 29, 2019

Sounds like it would break existing versions so major bump https://semver.org/.

Unless you put it behind a flag, then minor bump.

Then in the next major release you could remove the flag and make it the default behavior

@awm33
Copy link
Copy Markdown
Member

awm33 commented Aug 29, 2019

@AlexanderMann @airhorns @aroder I don't really have a strong release opinion, but it seems the options are:

  1. Just merging it and doing minor release, it seems like only @airhorns was using it anyway?
  2. Merge and make it conditional to a config options, doing a minor release
  3. Major release and sticking true to the letter of semver

I just grab the latest version by default for most of my ad-hoc work, then lock in for client prod pipelines, so it all works fine for our work.

@airhorns
Copy link
Copy Markdown
Contributor Author

airhorns commented Aug 29, 2019

My guess is that the major version bump would cause more pain for users who have to dive in and figure out what the breaking change was, which in most cases would be for a brand new feature they didn't use before, so I'd say a minor change would be the pragmatic approach. Definitely violates the letter of the semver spec but the spec also talks about how major version 0 is special and different and should allow big changes and less of an expectation of stability.

@AlexanderMann
Copy link
Copy Markdown
Collaborator

I'm voting for putting this into a new minor release as this isn't really a patch, and I think anyone who is leveraging this functionality...is already finding it "broken". I'll wait till end of week to release in case others have strong opinions, but in my mind this would end up in v0.2.0

@AlexanderMann AlexanderMann merged commit c401fe9 into datamill-co:master Sep 9, 2019
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.

Confusion around STATE messages

4 participants