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

fix issue events schema #102

Merged
merged 4 commits into from
Feb 5, 2021
Merged

Conversation

daigotanaka
Copy link
Contributor

Description of change

Some of the properties were marked as "null" type.
Change them to nullable string type.

Manual QA steps

Before the fix, the tap was throwing an exception with the null type issue.
After the fix, the tap correctly fetches the data and stdout.

Risks

  • None since the scope is limited to the previously broken issue_events stream.

Rollback steps

  • revert this branch

Some of the properties were marked as "null" type.
Change them to nullable string type.
@cmerrick
Copy link
Contributor

cmerrick commented Jan 8, 2021

Hi @daigotanaka, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@daigotanaka
Copy link
Contributor Author

@cmerrick: Done signing the agreement.

@cmerrick
Copy link
Contributor

cmerrick commented Jan 9, 2021

You did it @daigotanaka!

Thank you for signing the Singer Contribution License Agreement.

@daigotanaka
Copy link
Contributor Author

daigotanaka commented Jan 9, 2021

Just realized this could be a duplicated effort with #97 to solve #95, except that I corrected other fields on top of description.

@luandy64
Copy link
Contributor

luandy64 commented Feb 4, 2021

@daigotanaka this looks good to me. I want to run some tests tomorrow, and then I can merge

@@ -171,7 +171,8 @@
},
"milestone": {
"type": [
"null"
"null",
"string"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong.

I made an issue on this repo (#104) and the milestone there returned as an object:

'milestone': {'title': 'new-feature'}

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I was looking at the wrong milestone, but it's still an object:

           'milestone': {'closed_at': None,
                         'closed_issues': 0,
                         'created_at': '2021-02-05T15:49:29Z',
                         'creator': {'avatar_url': 'https://avatars.githubusercontent.com/u/19473996?v=4',
                                     'events_url': 'https://api.github.com/users/luandy64/events{/privacy}',
                                     'followers_url': 'https://api.github.com/users/luandy64/followers',
                                     'following_url': 'https://api.github.com/users/luandy64/following{/other_user}',
                                     'gists_url': 'https://api.github.com/users/luandy64/gists{/gist_id}',
                                     'gravatar_id': '',
                                     'html_url': 'https://github.com/luandy64',
                                     'id': 19473996,
                                     'login': 'luandy64',
                                     'node_id': 'MDQ6VXNlcjE5NDczOTk2',
                                     'organizations_url': 'https://api.github.com/users/luandy64/orgs',
                                     'received_events_url': 'https://api.github.com/users/luandy64/received_events',
                                     'repos_url': 'https://api.github.com/users/luandy64/repos',
                                     'site_admin': False,
                                     'starred_url': 'https://api.github.com/users/luandy64/starred{/owner}{/repo}',
                                     'subscriptions_url': 'https://api.github.com/users/luandy64/subscriptions',
                                     'type': 'User',
                                     'url': 'https://api.github.com/users/luandy64'},
                         'description': None,
                         'due_on': None,
                         'html_url': 'https://github.com/singer-io/tap-github/milestone/1',
                         'id': 6398418,
                         'labels_url': 'https://api.github.com/repos/singer-io/tap-github/milestones/1/labels',
                         'node_id': 'MDk6TWlsZXN0b25lNjM5ODQxOA==',
                         'number': 1,
                         'open_issues': 1,
                         'state': 'open',
                         'title': 'new-feature',
                         'updated_at': '2021-02-05T15:49:48Z',
                         'url': 'https://api.github.com/repos/singer-io/tap-github/milestones/1'},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luandy64 Thx for reviewing. My sample data didn't have milestones. I parsed your example and created the (sub) schema using https://pypi.org/project/getschema/

Your example contained due_on: None, so I manually changed it to date-time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, the bane of my existence is creating test data to validate these schemas

@luandy64 luandy64 merged commit 7401005 into singer-io:master Feb 5, 2021
sherifnada added a commit to airbytehq/tap-github that referenced this pull request Feb 13, 2021
This was referenced Apr 6, 2021
AJWurts pushed a commit to villagelabsco/tap-github that referenced this pull request Oct 24, 2024
* fix issue events schema

Some of the properties were marked as "null" type.
Change them to nullable string type.

* fix milestone property

Closes singer-io#95, singer-io#97
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.

3 participants