@edgarrmondragon any thoughts on questions 1 and 2 above?

@edgarrmondragon any thoughts on questions 1 and 2 above?

@ericboucher the schema and replication keys look the same, so the rest and graphql versions only differ in the pagination order?

the graphql endpoint is missing gravatar_id for the user object, is it even worth mentioning? (I lean towards no)

I agree 👍

The replication keys are different owner, repo -> repo_id @edgarrmondragon

@ericboucher I see the primary key will change with the new graphql stream. What do you think of the following plan:

• Change the name of the new stream to something like stargazers_graphql
• Add a deprecation warning, in the readme and maybe in the code too for the REST version of the stream
• Deselect the new version by default (TBD how we handle that)
• Release new version with both streams

In a future release we remove the REST version and consider renaming the graphql one.

cc @aaronsteers

@edgarrmondragon not sure I'm a fan since the next release would be yet another breaking change by renaming thisstargazers_graphql to stargazers for consistency.

To be honest, since the previous stream was working so poorly I think it's reasonable to introduce this one and override the previous one directly. Maybe we can add a warning in the code on this streams initialization? Is there a way to force the target to drop existing tables / update indexes?

@edgarrmondragon @aaronsteers I followed your suggestion and used the same partition keys so that we can "safely" override the REST stream completely. Which I did in the end.

The breaking id changes should happen in #122

Please have a final look and let me know what you think.

cc @laurentS for final thoughts as well

To be honest, since the previous stream was working so poorly I think it's reasonable to introduce this one and override the previous one directly. Maybe we can add a warning in the code on this streams initialization? Is there a way to force the target to drop existing tables / update indexes?

I don't think there's any defined behaviour on the target side, each target can do pretty much anything with the data, at least from the singer spec perspective, which just defines the interface between tap and target.
But I'd agree with the view that the current stream is behaving poorly. Also, taking into account the number of stars on this repo (5, of which 2 are commenting on this PR and 1 is with Meltano), I'd think it's fair to assume the number of users is very low (and those actually using this stream even lower).
Considering the sort of problems an unstable PKey can bring (see #110 and #112), I'd be in favour of simply replacing the old stream with the new one (and stable keys), with a big fat warning in the logs on init. As an option, rename the old stream stargazers_rest and keep it with a deprecation warning for a bit longer, so that on the off chance that someone is actually using the stream, they have an easy fix.

Is it easy to use a stream with another name? Eg. use stargazers_rest but still write to a stargazers table? Is this something meltano-cli provides @edgarrmondragon ?

Is it easy to use a stream with another name? Eg. use stargazers_rest but still write to a stargazers table? Is this something meltano-cli provides @edgarrmondragon ?

@ericboucher the SDK comes with stream renaming out-of-the-box: https://sdk.meltano.com/en/latest/stream_maps.html#aliasing-a-stream-using-alias

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication