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 variable nanosecond timestamp update_time from response #4819

Conversation

chemelnucfin
Copy link
Contributor

@tswast Thanks for the hint.

Closes #4807

@chemelnucfin chemelnucfin added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: runtimeconfig Issues related to the Cloud Runtime Config API API. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jan 31, 2018
@chemelnucfin chemelnucfin self-assigned this Jan 31, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2018
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

LGTM, though we might need a fallback in case microseconds appear again.

We should check with the product team to see what they are committing to.

@chemelnucfin
Copy link
Contributor Author

Would a try except work better then or should we just wait?

try:
  return ms
except ValueError:
  return ns

@tswast
Copy link
Contributor

tswast commented Feb 1, 2018

I'm emailing the team to check.

@chemelnucfin chemelnucfin changed the title Runtime Config: Fix variable nanosecond timestamp update_time from response Fix variable nanosecond timestamp update_time from response Feb 20, 2018
@tseaver tseaver removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Feb 20, 2018
@tseaver
Copy link
Contributor

tseaver commented Mar 5, 2018

@tswast ping?

@tswast
Copy link
Contributor

tswast commented Mar 5, 2018

I never got a response from the folks I've contacted before on the RuntimeConfig team.

@mgannholm Can you comment on this?

@tswast
Copy link
Contributor

tswast commented Mar 6, 2018

Okay, I got confirmation from Victor on the RuntimeConfig team that "it might change from platform to platform". We need to be able to handle both.

Yes, try-except would be a good way to do it. Alternatively, I believe we have places (in BigQuery, maybe?) where we try to parse the string a little bit to see how many digits we have before trying to parse.

@chemelnucfin
Copy link
Contributor Author

chemelnucfin commented Mar 6, 2018

I'll wait until the datetime PRs #4979, #4980, #4981 are merged before finishing this up.

@chemelnucfin chemelnucfin force-pushed the issue4807-runtime_config_nanoseconds branch 2 times, most recently from 7432b55 to 7cd4bee Compare March 25, 2018 18:01
@chemelnucfin chemelnucfin force-pushed the issue4807-runtime_config_nanoseconds branch from 7cd4bee to 2dc2357 Compare April 4, 2018 22:04
@chemelnucfin chemelnucfin force-pushed the issue4807-runtime_config_nanoseconds branch from 2dc2357 to 8f38fee Compare April 4, 2018 22:19
@chemelnucfin
Copy link
Contributor Author

@tswast PTAL at your convenience.


VARIABLE_NAME = 'my-variable/abcd'
VARIABLE_PATH = '%s/variables/%s' % (self.CONFIG_PATH, VARIABLE_NAME)
RESOURCE = {
'name': VARIABLE_PATH,
'value': 'bXktdmFyaWFibGUtdmFsdWU=', # base64 my-variable-value
'updateTime': '2016-04-14T21:21:54.5000Z',
'updateTime': '2016-04-14T21:21:54.123456789Z',

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@chemelnucfin
Copy link
Contributor Author

going to merge this and diff from this in the new PR. (I have it written already)

@chemelnucfin chemelnucfin merged commit 9a6acfa into googleapis:master Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: runtimeconfig Issues related to the Cloud Runtime Config API API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants