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.

Sign up for GitHub

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

Jump to bottom

Update logging codegen #1629

Closed
wants to merge 3 commits into from
Closed

Update logging codegen #1629

wants to merge 3 commits into from

Conversation

geigerj
Copy link
Contributor

@geigerj geigerj commented Mar 17, 2016

Reflects updates to the code generation made between January and now.

An sample usage snippet is available here: https://gist.github.com/geigerj/de050b53f8887e9d7a07

@tbetbetbe
@anthmgoogle
@jgeewax

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 17, 2016
@tbetbetbe
Copy link

@jcanizales

dhermes
dhermes reviewed Mar 18, 2016
View reviewed changes
gcloud/logging/config_service_v2_api.py
@@ -14,64 +14,123 @@
#
# EDITING INSTRUCTIONS
# This file was generated from the file
# https://github.com/google/googleapis/blob/7710ead495227e80a0f06ceb66bdf3238d926f77/google/logging/v2/logging_config.proto,
# https://github.com/google/googleapis/blob/master/google/logging/v2/logging_config.proto,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Mar 18, 2016

@geigerj thanks for the new PR! I'll skip the detail-oriented review since @dhermes is tackling that bit, but just wanted to let you know that looking at the generated wrappers for the protobuf/gRPC API is quite illuminating, as I'm working on the REST/JSON wrappers this month.

This was referenced Mar 18, 2016
Open
Closed
dhermes
dhermes reviewed Mar 18, 2016
View reviewed changes
gcloud/logging/config_service_v2_api.py
if app_name is None:
app_name = 'gax'
if app_version is None:
app_version = google.gax.__version__

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2016

Design question. If the code is generated, couldn't you use the contents of the YAML file to just populate variables in the module? It seems that having to bear the weight of file I/O and parsing YAML is unneeded.

dhermes
dhermes reviewed Mar 18, 2016
View reviewed changes
gcloud/logging/logging_service_v2_api.py
'https://www.googleapis.com/auth/logging.admin',
'https://www.googleapis.com/auth/logging.read',
'https://www.googleapis.com/auth/cloud-platform.read-only',
'https://www.googleapis.com/auth/cloud-platform', )

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tbetbetbe tbetbetbe mentioned this pull request Mar 18, 2016
Closed
dhermes
dhermes reviewed Mar 18, 2016
View reviewed changes
gcloud/logging/logging_service_v2_api.py
google_apis_agent = '{}-{}/{}/gax-{}/{}'.format(
app_name, app_version, self._CODE_GEN_NAME_VERSION,
google.gax.__version__,
'python-{}'.format(platform.python_version()))

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2016

OK I finished my comments.

@tbetbetbe tbetbetbe mentioned this pull request Mar 18, 2016
Closed
@theacodes
Copy link
Contributor

Design question. If the code is generated, couldn't you use the contents of the YAML file to just populate variables in the module? It seems that having to bear the weight of file I/O and parsing YAML is unneeded.

I feel the same way. There seems to be no real benefit to doing it this way.

@tbetbetbe
Copy link

Design question. If the code is generated, couldn't you use the contents of the YAML file to just populate variables in the module? It seems that having to bear the weight of file I/O and parsing YAML is unneeded.
I feel the same way. There seems to be no real benefit to doing it this way.

In the not too far-off future, the plan is to allow user configuration of api settings via yaml

  • the exact mechanism is still under discussion
  • hopefully, this will look a bit like application default creds, e.g,
    • there is a yaml file in the package will contain fallback config values
    • users will be specify an override either by specifying an environment variable or by copying to the settings to a well-known location

This was referenced Mar 18, 2016
Closed
Closed
@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2016

Having a private key in it's own file is a security issue.

Having user-overrides as code doesn't seem to be a problem.

MyService(my_override='foo', your_setting='nope_mine', ...)

@tbetbetbe tbetbetbe mentioned this pull request Mar 18, 2016
Closed
@jgeewax jgeewax added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 22, 2016
jgeewax
jgeewax reviewed Mar 22, 2016
View reviewed changes
gcloud/logging/config_service_v2_api.py
# merge preserves those additions if the generated source changes.

import os
import platform

from google.gax import api_callable

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@geigerj
Update codegen in response to PR feedback
660bacd 
Updates in response to feedback from PR:
  #1629

Fixes issues:
* googleapis/gax-python#57: Incorrect comment referred to a domain
  name as a "DNS"
* googleapis/gax-python#58: Incorrect proto was used in codegen,
  leading to a dangling link in the generated comments
* googleapis/gax-python#59: Simplify the format string for the user-
  agent header
* googleapis/gax-python#60: Use `{}` to initialize dictionaries
* googleapis/gax-python#61: Use `pkg_resources` to load the client
  config file
jgeewax
jgeewax reviewed Mar 22, 2016
View reviewed changes
gcloud/logging/config_service_v2_api.py
port=DEFAULT_SERVICE_PORT,
channel=None,
ssl_creds=None,
scopes=_ALL_SCOPES,

This comment was marked as spam.

Sign in to view

bjwatson pushed a commit to googleapis/gapic-generator that referenced this pull request Apr 20, 2016
@geigerj
Updates from GitHub PR
5eeb9aa 
* Use {} rather than dict()
* Use pkg_resources to load service config file
* Simplify format string for tracking header and change separator to
  ';' instead of '/'.
* Fix incorrect comment where "DNS" was used although "domain name"
  was meant

See:
  googleapis/google-cloud-python#1629

Pre-push hook installed.

Change-Id: I02c832dfcd34eed8f79338711d6e2d0e34cf0182
@tseaver
Copy link
Contributor

tseaver commented Jun 15, 2016

AFAIK, the generated code will land in a separate gax-google-logging-v2 package (under the google namespace)

@tseaver tseaver closed this Jun 15, 2016
vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
@nnegrey @busunkim96
Moving the Dialogflow samples to the python-docs-sampl… [(#1629)](Goo…
99760ee 
…gleCloudPlatform/python-docs-samples#1629)

* [DO_NOT_MERGE] Moving the Dialogflow samples to the python-docs-samples repo

* Add missing region tags for tracking

* Style updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@geigerj @tbetbetbe @tseaver @dhermes @theacodes @broady @jgeewax @googlebot