New issue
Advanced search Search tips

Issue 771503 link

Starred by 1 user
Status: Fixed
Owner:
tandrii@chromium.org
Closed: Nov 2017
Cc:
tandrii@chromium.org
no...@chromium.org
vadimsh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

do not require schema in buildbucket server field

Project Member Reported by no...@chromium.org, Oct 4 2017 
BuildbucketTask.server field in cron.proto is URL, which means URL Schema, path and other parts of URL are configurable. Only hostname should be.

I think it can be implemented without breaking changes:
1. start accepting just hostname
2. update docs, say it is a hostname, not URL
3. update configs 
4. stop accepting URL

perhaps we should do it soon or never (would be hard)

Comment 1 by tandrii@chromium.org, Oct 4 2017

Owner: tandrii@chromium.org
Status: Assigned (was: Untriaged)
Overall, I agree that just host is better.

Now, we already accept just host as well as empty path part: https://cs.chromium.org/chromium/infra/go/src/go.chromium.org/luci/scheduler/appengine/task/buildbucket/buildbucket_test.go?type=cs&sq=package:chromium&l=50

Here is doc change: https://chromium-review.googlesource.com/#/c/infra/luci/luci-go/+/700823

Validation errors still reference URL, but it doesn't matter because those errors are at the moment only visible in AE logs.
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/40a99a0b3b09d1dbf38ef206c2c0922ba9a72d7e

commit 40a99a0b3b09d1dbf38ef206c2c0922ba9a72d7e
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Wed Oct 04 21:21:41 2017

scheduler: update doc in config.

buildbucket server should be just a host, though we still accept URL.

R=vadimsh@chromium.org

Bug:  771503 
Change-Id: If226145404d5b4720e60b055c68b7cf5873ae7fe
Reviewed-on: https://chromium-review.googlesource.com/700823
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

[modify] https://crrev.com/40a99a0b3b09d1dbf38ef206c2c0922ba9a72d7e/scheduler/appengine/messages/cron.pb.go
[modify] https://crrev.com/40a99a0b3b09d1dbf38ef206c2c0922ba9a72d7e/scheduler/appengine/messages/cron.proto

Comment 3 by tandrii@chromium.org, Oct 31 2017

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 31 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e

commit ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Tue Oct 31 20:16:42 2017

scheduler: provide context when validating task configs.

This is so that one can easily use loging.Warnf(), which is useful
today to deprecate some config features.

Bug:  771503 
Change-Id: I305e27b730d280b8446fe04dc00271ff16dbb252
Reviewed-on: https://chromium-review.googlesource.com/747366
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>

[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/catalog/catalog.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/catalog/catalog_test.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/engine/controller.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/engine/engine_test.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/presentation/state.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/buildbucket/buildbucket.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/buildbucket/buildbucket_test.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/gitiles/gitiles.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/gitiles/gitiles_test.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/noop/noop.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/task.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/urlfetch/urlfetch.go
[modify] https://crrev.com/ddd7e2d5e0e4da988e53ff6cf97b927848ddf64e/scheduler/appengine/task/urlfetch/urlfetch_test.go
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 31 2017

Labels: merge-merged-config
The following revision refers to this bug:
  https://chromium.googlesource.com/infra/experimental/+/8383248d6b3ded1d7c670a4368df7f8c6aeeca31

commit 8383248d6b3ded1d7c670a4368df7f8c6aeeca31
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Tue Oct 31 22:32:56 2017

scheduler: fix buildbucket server.

TBR=vadimsh@chromium.org

Bug:  771503 
Change-Id: I3d977279d056959c465d17ae695357f59a69a35c
Reviewed-on: https://chromium-review.googlesource.com/747887
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/8383248d6b3ded1d7c670a4368df7f8c6aeeca31/luci-scheduler-dev.cfg
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 31 2017 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infra/infra_internal/+/7ac8d17beb144e64482ab48c77d01ec71e711ede

commit 7ac8d17beb144e64482ab48c77d01ec71e711ede
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Tue Oct 31 22:43:33 2017
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 31 2017 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/franky/data/+/4afcdc5f05e857645d5f26321dc48c6d829a4bea

commit 4afcdc5f05e857645d5f26321dc48c6d829a4bea
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Tue Oct 31 22:53:04 2017
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/external/gyp/+/e5356830e42dd5f2edc0b68e05646c658e19e25c

commit e5356830e42dd5f2edc0b68e05646c658e19e25c
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Wed Nov 01 00:02:31 2017

scheduler: buildbucket server is just a host now.

R=dpranke@chromium.org, vadimsh@chromium.org

Bug:  chromium:771503 
Change-Id: I240334d09d15cae7ac6200eba5dbad62b6073aba
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/748001
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/e5356830e42dd5f2edc0b68e05646c658e19e25c/luci-scheduler.cfg

Comment 13 by tandrii@chromium.org, Nov 1 2017 

All configs migrated.
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/4fce7a4f5e6dfc6837fcf225d0cb2d346b3c3242

commit 4fce7a4f5e6dfc6837fcf225d0cb2d346b3c3242
Author: Andrii Shyshkalov <tandrii@chromium.org>
Date: Wed Nov 01 00:46:22 2017

scheduler: stop accepting URLs as buildbucket server fields.

Bug:  771503 
Change-Id: Ia805d0764222046ba5b52d4d591405873579b25a
Reviewed-on: https://chromium-review.googlesource.com/747216
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/4fce7a4f5e6dfc6837fcf225d0cb2d346b3c3242/scheduler/appengine/task/buildbucket/buildbucket.go
[modify] https://crrev.com/4fce7a4f5e6dfc6837fcf225d0cb2d346b3c3242/scheduler/appengine/task/buildbucket/buildbucket_test.go

Comment 15 by tandrii@chromium.org, Nov 1 2017

Status: Fixed (was: Started)
Deployed to dev and prod as of version 2249-4fce7a4. It's now an error to specify anything but host in buildbucket server.

Comment 16 by no...@chromium.org, Nov 3 2017 

Thanks
►

Sign in to add a comment