Skip to content

feat: python version of idp-sql tutorial #5315

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

Merged
merged 78 commits into from
Feb 25, 2021
Merged

feat: python version of idp-sql tutorial #5315

merged 78 commits into from
Feb 25, 2021

Conversation

glasnt
Copy link
Contributor

@glasnt glasnt commented Jan 31, 2021

Description

Checklist

@glasnt glasnt requested review from averikitsch, grant and a team as code owners January 31, 2021 23:34
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Jan 31, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2021
@snippet-bot
Copy link

snippet-bot bot commented Jan 31, 2021

Here is the summary of changes.

You are about to add 3 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

Copy link
Contributor

@averikitsch averikitsch left a comment

Choose a reason for hiding this comment

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

A few thoughts:

  1. Is there an idiomatic way to split main.py into separate files to reduce complexity?
  2. A priority is to add a sig term handler to show of that functionality (depending on this postgres library, the db connections can be closed when sigterm is caught)
  3. A lesser priority would be to implement better logging. In order for stackdriver to parse the logs correctly they need to be json formatted with a severity property to set for the log level to be parsed.

@glasnt glasnt mentioned this pull request Feb 3, 2021
@glasnt
Copy link
Contributor Author

glasnt commented Feb 25, 2021

As discussed with @averikitsch, this now uses NullPool when testing only to work around service delete issues; will re-address when upstream changes happen around those. I'm comfortable with this PR being merged as it is.

(I have the ability to hit the [squash and merge] button with the approvals received, but I'm not sure if I should be merging my own PRs)

@averikitsch
Copy link
Contributor

As discussed with @averikitsch, this now uses NullPool when testing only to work around service delete issues; will re-address when upstream changes happen around those. I'm comfortable with this PR being merged as it is.

(I have the ability to hit the [squash and merge] button with the approvals received, but I'm not sure if I should be merging my own PRs)

LGTM. Feel free to merge

@glasnt glasnt merged commit 16941a1 into GoogleCloudPlatform:master Feb 25, 2021
@glasnt glasnt deleted the idp-tutorial branch February 25, 2021 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants