Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108073002:
bot_update: Allow patch_oauth2 to work in kitchen (Closed)
|
Created:
4 years, 5 months ago by martiniss Modified:
4 years, 5 months ago Reviewers:
Vadim Sh. CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptionbot_update: Allow patch_oauth2 to work in kitchen
site config is not present on builders which use kitchen, so we need to use credentials provided by puppet in the /creds directory.
BUG=624212
TBR=hinoka
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/575e6766f2eeab4d3f8b2f0d57fa0fda519f4369
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments. #
Total comments: 4
Patch Set 3 : Nits #Messages
Total messages: 20 (9 generated)
Description was changed from ========== bot_update: Allow patch_ouath2 to work in kitchen BUG=624212 ========== to ========== bot_update: Allow patch_ouath2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet. BUG=624212 ==========
martiniss@chromium.org changed reviewers: + vadimsh@chromium.org
https://codereview.chromium.org/2108073002/diff/1/recipe_modules/bot_update/a... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2108073002/diff/1/recipe_modules/bot_update/a... recipe_modules/bot_update/api.py:66: patch_oauth2_in_puppet: If the oauth2 credentials live in puppet, or in this is misleading, in both cases they are managed by puppet. Perhaps 'use_site_config_creds' or something. Or even PATCH_CREDS_NONE = 'None' PATCH_CREDS_SITE_CONFIG = 'site-config' PATCH_CREDS_DEFAULT = 'default' And convert path_oauth2 to be a enumeration with PATCH_CREDS_* possible values. Well, whatever. https://codereview.chromium.org/2108073002/diff/1/recipe_modules/bot_update/a... recipe_modules/bot_update/api.py:126: email_file = '/creds/refresh_tokens/rietveld_client_email' this is going to be /creds/rietveld/client_email /creds/rietveld/secret_key
https://codereview.chromium.org/2108073002/diff/1/recipe_modules/bot_update/a... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2108073002/diff/1/recipe_modules/bot_update/a... recipe_modules/bot_update/api.py:66: patch_oauth2_in_puppet: If the oauth2 credentials live in puppet, or in On 2016/06/29 at 03:58:08, Vadim Sh. wrote: > this is misleading, in both cases they are managed by puppet. > > Perhaps 'use_site_config_creds' or something. Or even > > PATCH_CREDS_NONE = 'None' > PATCH_CREDS_SITE_CONFIG = 'site-config' > PATCH_CREDS_DEFAULT = 'default' > > And convert path_oauth2 to be a enumeration with PATCH_CREDS_* possible values. > > Well, whatever. went with 'use_site_config_creds' https://codereview.chromium.org/2108073002/diff/1/recipe_modules/bot_update/a... recipe_modules/bot_update/api.py:126: email_file = '/creds/refresh_tokens/rietveld_client_email' On 2016/06/29 at 03:58:08, Vadim Sh. wrote: > this is going to be > > /creds/rietveld/client_email > /creds/rietveld/secret_key Done.
lgtm with nits also please fix type in CL title and description https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:66: patch_oauth2_in_puppet: If the oauth2 credentials live in puppet, or in update (or remove, since Args section seems to be missing majority of arguments anyway) https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:131: email_file = '/creds/rietveld/client_email' this won't work on Windows btw, its C:\Creds\ on windows. We can fix this later though. We probably needs m.path['creds'] config or something like that.
On 2016/06/29 04:35:02, Vadim Sh. wrote: > lgtm with nits > > also please fix type in CL title and description > err.. typo >_< > https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... > File recipe_modules/bot_update/api.py (right): > > https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... > recipe_modules/bot_update/api.py:66: patch_oauth2_in_puppet: If the oauth2 > credentials live in puppet, or in > update (or remove, since Args section seems to be missing majority of arguments > anyway) > > https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... > recipe_modules/bot_update/api.py:131: email_file = > '/creds/rietveld/client_email' > this won't work on Windows btw, its C:\Creds\ on windows. > > We can fix this later though. We probably needs m.path['creds'] config or > something like that.
Description was changed from ========== bot_update: Allow patch_ouath2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet. BUG=624212 ========== to ========== bot_update: Allow patch_oauth2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet in the /creds directory. BUG=624212 ==========
The CQ bit was checked by martiniss@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/29 04:36:10, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... So, no nits fixing ? :(
On 2016/06/29 at 04:38:11, vadimsh wrote: > On 2016/06/29 04:36:10, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > So, no nits fixing ? :( Oh sorry, your double message confused me D:
The CQ bit was unchecked by martiniss@chromium.org
Description was changed from ========== bot_update: Allow patch_oauth2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet in the /creds directory. BUG=624212 ========== to ========== bot_update: Allow patch_oauth2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet in the /creds directory. BUG=624212 TBR=hinoka ==========
Ok, responded to nits. https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... File recipe_modules/bot_update/api.py (right): https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:66: patch_oauth2_in_puppet: If the oauth2 credentials live in puppet, or in On 2016/06/29 at 04:35:02, Vadim Sh. wrote: > update (or remove, since Args section seems to be missing majority of arguments anyway) Updated. Trying to document stuff as we add it. https://codereview.chromium.org/2108073002/diff/20001/recipe_modules/bot_upda... recipe_modules/bot_update/api.py:131: email_file = '/creds/rietveld/client_email' On 2016/06/29 at 04:35:02, Vadim Sh. wrote: > this won't work on Windows btw, its C:\Creds\ on windows. > > We can fix this later though. We probably needs m.path['creds'] config or something like that. Ok, added TODO for myself :/
(TBR hinoka for OWNERS)
The CQ bit was checked by martiniss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2108073002/#ps40001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== bot_update: Allow patch_oauth2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet in the /creds directory. BUG=624212 TBR=hinoka ========== to ========== bot_update: Allow patch_oauth2 to work in kitchen site config is not present on builders which use kitchen, so we need to use credentials provided by puppet in the /creds directory. BUG=624212 TBR=hinoka Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/575e6766f2eeab... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/575e6766f2eeab... |
