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

Google Slides & Google Drive #1031

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

Conversation

elyse-weiss
Copy link
Contributor

Closes #957

Notes

  • This is my first time writing tests! They did seem to pass
  • I was not able to run docs successfully, so I am not sure my new functions made it into docs

@elyse-weiss elyse-weiss requested a review from shaunagm April 3, 2024 20:23
Copy link
Collaborator

@shaunagm shaunagm left a comment

Choose a reason for hiding this comment

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

This looks great overall, but we need to make sure we've got proper test coverage and handle any errors that those tests raise. Let me know if you need help figuring out the test coverage angle.


@unittest.skipIf(
not os.environ.get("LIVE_TEST"), "Skipping because not running live test"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You've set this up so that all of your tests will be skipped unless the environmental variable is set to LIVE_TEST. It's great to have live tests, but we also want tests using Mocks that can be run as part of the regular automated testing pipeline. Can you also write some mock versions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, for the live tests, will everyone have access to the test slides you've created? If not, instead of hard coding that variable into the tests, we likely want to create another environmental variable people can use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, for the live tests, will everyone have access to the test slides you've created? If not, instead of hard coding that variable into the tests, we likely want to create another environmental variable people can use.

YES the Test Slides are public to everyone on the internet (they contain truly nothing of value). I think given the nature of the tests - in terms of replacing certain text and images, we need to use a standard default.

test/test_google/test_google_drive.py Outdated Show resolved Hide resolved
@shaunagm
Copy link
Collaborator

shaunagm commented Apr 4, 2024

re: the docs, I don't see any updates to the docs. You'll want to edit the google.rst file to include docs on your new connectors. You can use the sections that are already there (ie Google Sheets, Google Admin) as a template. You can also use these instructions as a guide, although your case is a little different because you're not creating a completely new connector, so you don't need to create a completely new rst file for it, or add it to the sidebar. Just updated the google.rst doc should work.

Building the docs locally is a little tricky, you can follow these steps - a common issue is people not realizing they need to go into the docs folder and install some docs-specific requirements before trying to build the docs.

@elyse-weiss elyse-weiss requested a review from shaunagm May 7, 2024 20:15
@shaunagm
Copy link
Collaborator

Weirdly only two of the dozen or so tests is being run (both passing, though!) and there are merge conflicts. @elyse-weiss would you be up for pairing on this sometime soon, and we can work through the issues together and get this merged?

@elyse-weiss
Copy link
Contributor Author

@shaunagm happy to pair! would love to get this merged :)

@elyse-weiss
Copy link
Contributor Author

@shaunagm tests ran successfully locally!

Comment on lines +30 to +38
setup_google_application_credentials(
google_keyfile_dict, "GOOGLE_DRIVE_CREDENTIALS"
)
google_credential_file = open(os.environ["GOOGLE_DRIVE_CREDENTIALS"])
credentials_dict = json.load(google_credential_file)

credentials = Credentials.from_service_account_info(
credentials_dict, scopes=scope, subject=subject
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you consider using the new pattern adopted by other google connectors as of #1040 to avoid reimplementing the issue #1039?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@austinweisgrau I previously had:

self.client = build("drive", "v3", credentials=credentials)

If I follow your code from the other PR, will the following work too:

self.client = AuthorizedSession(credentials)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments on google_drive apply to this connector

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.

[Feature/Addition] Google Slides Connector
3 participants