MeltanoLabs / tap-gitlab

Singer.io Tap for extracting data from Gitlab's API
GNU Affero General Public License v3.0
11 stars 27 forks source link

When no catalog is provided, no streams are selected, which is backward incompatible #46

Closed pnadolny13 closed 2 years ago

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 10:46

As seen in https://gitlab.com/rabidaudio/meltano/-/jobs/1019557357.

This ~bug was introduced by https://gitlab.com/meltano/tap-gitlab/-/merge_requests/34, released as v0.9.12.

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 10:46

mentioned in merge request meltano!2024

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 10:46

changed the description

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 10:49

We should modify the not args.catalog branch at https://gitlab.com/meltano/tap-gitlab/-/blob/f41bc2f165a1f4ba32417499b9abf477bedd6abf/tap_gitlab/__init__.py#L873 to mark all streams as selected: true that have inclusion: available

Edit: or just add a selected=True arg to do_discover that will set selected: true on each stream.

Edit: We should do this not just for streams, but properties as well, I assume

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 10:49

assigned to @rabidaudio

pnadolny13 commented 2 years ago

In GitLab by @rabidaudio on Feb 10, 2021, 10:59

We really just want the catalog to select all streams by default, right? Something like:

def do_discover():
    streams = []
    for resource, config in RESOURCES.items():
        mdata = metadata.get_standard_metadata(
            schema=config["schema"],
            key_properties=config["key_properties"],
            valid_replication_keys=config.get("replication_keys"),
            replication_method=config["replication_method"],
        )

        mdata = metadata.to_list(metadata.write(metadata.to_map(mdata), (), 'selected', True))

        # ....

Meltano and others can still edit the catalog to unselect stuff.

I would have thought this was the default behavior of singer-python :upside_down: /shrug

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 11:07

@rabidaudio Yeah that works, just selecting by default always, except for the "unsupported" streams.

I don't know if we should also add selected: true to every properties.

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 11:09

mentioned in merge request !35

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 11:09

mentioned in commit 602e89fba0d4c3f31307ea5fe03a2d3f37dbab3b

pnadolny13 commented 2 years ago

In GitLab by @rabidaudio on Feb 10, 2021, 11:12

Well from the singer docs:

Streams shouldn’t be selected by default. They must have an inclusion: available property

given that, it's probably better to only select all if no catalog is supplied?

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 10, 2021, 11:15

@rabidaudio Works for me, since we mostly just want to do this for backward compatibility.

pnadolny13 commented 2 years ago

In GitLab by @rabidaudio on Feb 10, 2021, 13:19

mentioned in merge request !36

pnadolny13 commented 2 years ago

In GitLab by @DouweM on Feb 11, 2021, 12:14

mentioned in commit 95103ec271ac9372815b8c99c474e62d64b0708b