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

feat: create CustomPropertiesStream class #268

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

MindaugasN
Copy link

create CustomPropertiesStream class that fetches data from https://docs.github.com/en/rest/repos/custom-properties?apiVersion=2022-11-28

@edgarrmondragon
Copy link
Member

Seeing a 404 in the tests: Custom properties only supported for organizations.

Perhaps we should think of a way of skipping/deselecting this stream for non-org namespaces?

@MindaugasN
Copy link
Author

Seeing a 404 in the tests: Custom properties only supported for organizations.

Perhaps we should think of a way of skipping/deselecting this stream for non-org namespaces?

Is there already an example with that? And how to tell if a namespace is org / non-org?

@edgarrmondragon
Copy link
Member

Seeing a 404 in the tests: Custom properties only supported for organizations.
Perhaps we should think of a way of skipping/deselecting this stream for non-org namespaces?

Is there already an example with that? And how to tell if a namespace is org / non-org?

@MindaugasN I think moving it here

ORGANIZATIONS = (
{"organizations"},
[OrganizationStream, TeamMembersStream, TeamRolesStream, TeamsStream],
)

would be enough

@MindaugasN
Copy link
Author

MindaugasN commented Jul 8, 2024

I moved, but first of all, I am not sure, if I did that correctly.
What I see though, puting stream like this (under organizations) - it does not work for me anymore, CustomPropertiesStream should be under repository stream. Maybe you didn't mean that I should move CustomPropertiesStream under organization_streams.py?
Another thing, I don't understand why pre-commit.ci fails. Do you have an idea?

@edgarrmondragon
Copy link
Member

I moved, but first of all, I am not sure, if I did that correctly. What I see though, puting stream like this (under organizations) - it does not work for me anymore, CustomPropertiesStream should be under repository stream.

Can you say more about this? What do you mean it's not working anymore?

Maybe you didn't mean that I should move CustomPropertiesStream under organization_streams.py?

Not really. The stream class can stay in tap_github/repository_streams.py.

Another thing, I don't understand why pre-commit.ci fails. Do you have an idea?

I think you're missing a newline at the end of tap_github/organization_streams.py.

@MindaugasN
Copy link
Author

MindaugasN commented Jul 9, 2024

Not really. The stream class can stay in tap_github/repository_streams.py.

Got it, that solved all my problems :)

And with the test - I ran Black locally and formatted my code

@edgarrmondragon
Copy link
Member

I think we're back at #268 (comment) 😅

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