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.

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

Implement Discover mode #1

Merged
merged 14 commits into from
Jun 22, 2022
Merged

Implement Discover mode #1

merged 14 commits into from
Jun 22, 2022

Conversation

Phanatic
Copy link
Contributor

This PR implements the Discover Mode for a singer tap as described in the specification.

A lot of the code here looks similar because its been purloined from PlanetScale's Airbyte source.
The key difference being that the types we return from the database access types are Singer types, and not Airbyte.

Key Points

  1. Singer requires that all JSON output go to stdout, and all log & error messages go to stderr, which is why you see the logger accept an out and an stderr
  2. Singer requires that each property and type be selectable individually, which is why you see the metadata object array on a Stream type.

TODO

  • Add Tests

@Phanatic Phanatic requested a review from mdlayher June 22, 2022 17:35
Dockerfile Outdated
@@ -0,0 +1,19 @@
# syntax=docker/dockerfile:1

ARG GO_VERSION=1.18rc1
Copy link
Member

Choose a reason for hiding this comment

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

Should do 1.18.3, same for CI.

"strings"
)

// PlanetScaleSource defines a configured Airbyte Source for a PlanetScale database
Copy link
Member

Choose a reason for hiding this comment

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

Probably grep for uses of airbyte throughout and replace them.

return tables, errors.Wrap(err, "unable to iterate table rows")
}

return tables, err
Copy link
Member

Choose a reason for hiding this comment

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

All errors have already been checked so this is a little misleading, just return tables, nil.

func TableCursorToSerializedCursor(cursor *psdbconnect.TableCursor) (*SerializedCursor, error) {
d, err := codec.DefaultCodec.Marshal(cursor)
if err != nil {
return nil, errors.Wrap(err, "unable to marshal table cursor to save staate.")
Copy link
Member

Choose a reason for hiding this comment

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

*state, remove period for error chaining purposes.

Metadata NodeMetadata `json:"metadata"`
}

func (s *Stream) GenerateMetadata() error {
Copy link
Member

Choose a reason for hiding this comment

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

It's a little confusing to figure out what this function does since it implicitly requires certain fields to be set on Stream which it then reads and does updates with internally. Consider passing key properties as an explicit argument.

execute(discoverMode, configFilePath, catalogFilePath, stateFilePath)
}

func execute(discoverMode bool, configFilePath, catalogFilePath, stateFilePath string) {
Copy link
Member

Choose a reason for hiding this comment

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

Have this return an error and then do the log/os.Exit in main.

@Phanatic Phanatic merged commit 1d07b83 into main Jun 22, 2022
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.

2 participants