Skip to content

Fix variable nanosecond timestamp update_time from response - #4819

Merged
chemelnucfin merged 2 commits into
googleapis:masterfrom
chemelnucfin:issue4807-runtime_config_nanoseconds
Apr 4, 2018
Merged

Fix variable nanosecond timestamp update_time from response#4819
chemelnucfin merged 2 commits into
googleapis:masterfrom
chemelnucfin:issue4807-runtime_config_nanoseconds

Conversation

@chemelnucfin

Copy link
Copy Markdown
Contributor

@tswast Thanks for the hint.

Closes#4807

@chemelnucfinchemelnucfin 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
@chemelnucfinchemelnucfin self-assigned this Jan 31, 2018
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 31, 2018

@tswasttswast left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

try:
return ms
except ValueError:
return ns

@tswast

Copy link
Copy Markdown
Contributor

I'm emailing the team to check.

@chemelnucfinchemelnucfin changed the title Runtime Config: Fix variable nanosecond timestamp update_time from responseFix variable nanosecond timestamp update_time from responseFeb 20, 2018
@tseavertseaver 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
Copy Markdown
Contributor

@tswast ping?

@tswast

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

chemelnucfin commented Mar 6, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@chemelnucfin
chemelnucfinforce-pushed the issue4807-runtime_config_nanoseconds branch 2 times, most recently from 7432b55 to 7cd4beeCompareMarch 25, 2018 18:01
@chemelnucfin
chemelnucfinforce-pushed the issue4807-runtime_config_nanoseconds branch from 7cd4bee to 2dc2357CompareApril 4, 2018 22:04
@chemelnucfin
chemelnucfinforce-pushed the issue4807-runtime_config_nanoseconds branch from 2dc2357 to 8f38feeCompareApril 4, 2018 22:19
@chemelnucfin

Copy link
Copy Markdown
ContributorAuthor

@tswast PTAL at your convenience.

'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
Copy Markdown
ContributorAuthor

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

@chemelnucfin
chemelnucfin merged commit 9a6acfa into googleapis:masterApr 4, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: runtimeconfigIssues related to the Cloud Runtime Config API API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@chemelnucfin@tswast@tseaver@googlebot