🎉 GitHub Source: Don't minimize output fields

[cjwooo]

What

Our use case requires the full values in certain streams, such as the author and committer fields in the Commits stream, not just their ids.

How

Add an option to the GitHub Source spec, allowing the user to choose whether or not to minimize fields for all streams. By default, the option is true, meaning fields will still be minimized.

Pre-merge Checklist

Community member or Airbyter

• Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
• PR name follows PR naming conventions
• Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here

[sherifnada]

this is making me wonder if we should just not minimize fields at all. it seems to have been a fairly controversial move. I think I would prefer we do that since it's more in line with ELT. @cjwooo would you mind changing the PR to do that instead?

[cjwooo]

Yea I can do that. Do other sources have the same field minimization feature?

[sherifnada]

I don't think so, no

[cjwooo]

@sherifnada Should we get some confirmation from the larger community before unminimizing everything? I'm only concerned it would break existing users' workflows.

[sherifnada]

@cjwooo apologies - I should have been clearer. Totally agreed with you that backwards compatibility is desirable here. I think we should:

1. Keep the user_id field or any other minimized fields in records
2. Add back the expanded fields into the recods
   so it would be something like:

record['user_id'] = record['user']['id'] 

and we can get rid of the minimization logic. I believe this would be totally backwards compatible. Does that help?

[cjwooo]

That makes sense, but one issue I see is that part of the current minimization logic is transforming a list of objects into a list of ids: https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-github/source_github/streams.py#L200-L201. And the record key for this case stays the same, e.g. https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-github/source_github/schemas/pull_requests.json#L62-L66. I don't think we can suddenly switch the outputted records for the PullRequests stream to have an array of label objects under record['labels']

[cjwooo]

@sherifnada ^?

[sherifnada]

@cjwooo hmm that's a good point. I think we have a few options then:

1. keep the current schemas and in instances like what you describe, give the columns some other name which doesn't conflict (but that name would be out of line with what the API calls it, so we'd need to maintain that transformation forever)
2. Rip the bandaid and change the schema to what it's supposed to be, accepting that it would cause some breakage

I think I would rather do 2 rather than have a scar in the output schema for the rest of time. We could do a major version release to indicate this and manage the roll out such that consumers can prepare themselves for this. WDYT?

[cjwooo]

I'd also rather go with option 2. I'll go ahead and implement that.

[cjwooo]

@sherifnada Updated PR.

[sherifnada]

@cjwooo apologies to increase the scope on you here, but would you mind removing all minimization? if we're going to make a breaking change I would rather that we do it exactly once rather than phase out minimization in different stages.

Though if the current set of minimized fields will require no breaking changes in the future feel free to let it be and we'll finish that piece on our side. LMK.

Otherwise this looks great! thanks so much for trooping through this issue. I'll merge it as soon as you let me know the verdict on the above.

[cjwooo]

@sherifnada Removed all minimization logic.