scriptworker.yaml #23

Merged
merged 3 commits into from Nov 2, 2016

Projects

None yet

3 participants

@escapewindow
Member

I split out cot_config from config earlier to avoid an overly-complex json file. config_example.json has been wanting comments for a while, but hasn't been commented due to the restrictions of the format.

Moving to yaml allows us to combine the configs without too much complexity, since we can use comments in the example config.

This is a pretty large patch (another 61k patch) but a lot of it is tests and requirements-*prod.txt
I thought opening up a PR for just this piece might be easier than having this part of the entire chain of trust verification PR. If that's not true, I can close this PR and create one massive PR later.

@JohanLorenzo @lundjordan do you prefer thumb wrestling or do you want me to just choose one of you? :)

@escapewindow escapewindow scriptworker.yaml
We split out `cot_config` from `config` earlier to avoid an
overly-complex json file.  `config_example.json` has been wanting
comments for a while, due to the restrictions of the format.

Moving to yaml allows us to combine the configs without too much
complexity, since we can use comments in the example config.
03eb5b8
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling 03eb5b8 on escapewindow:yaml into b401d2f on mozilla-releng:master.

@JohanLorenzo

Shotgun! 😄

I love the comments in the config file! I don't think we require a bigger PR, the changes LGTM. Thanks!

@@ -0,0 +1,145 @@
+#-----------------------------------------------------------------------------------------------
+# Taskcluster worker settings.
+# For development, keep the test-dummy-* and dummy-worker-* convention.
@JohanLorenzo
JohanLorenzo Nov 2, 2016 Contributor

It so nice to be able to put comments in config files 👍 😃

+#-----------------------------------------------------------------------------------------------
+# Task configs
+#-----------------------------------------------------------------------------------------------
+artifact_expiration_hours: 24
@JohanLorenzo
JohanLorenzo Nov 2, 2016 Contributor

Out of curiosity, would it worth to nest the task configuration? For example:

task:
  artifact_expiration_hours: 24
  max_timeout: 1200

or

gpg:
  email: "scriptworker@example.com"

Edit: After going further in the PR, I realized this'll break too many things.

@escapewindow
escapewindow Nov 2, 2016 Member

Yeah, agreed that nesting would be nicer to read and organize, but it would require changing a lot of code, and would bump #24 in urgency.

+# Scriptworker paths.
+#-----------------------------------------------------------------------------------------------
+# Scriptworker logs go here; this is a long-lived directory.
+log_dir: "/tmp/log"
@JohanLorenzo
JohanLorenzo Nov 2, 2016 Contributor

If this is a long-lived dir, maybe we should use a different example than /tmp? What do you think of something like: /var/log/my_scriptworker_instance/

@escapewindow
escapewindow Nov 2, 2016 Member

I never intended /tmp to be a real location for anything but testing.
For puppetized instances, we're going with /builds/scriptworker/... and the like.
I'm not sure we gain that much from moving the log_dir for dev instances, and it forces someone testing to create /var/log/... and make sure it's writable for the current user.

I'm going to skip this suggestion for now, but if you feel strongly about it we can revisit.

scriptworker.yaml.tmpl
+# If `taskId` is specified in the path_regex, it must be in task.dependencies, the decision task,
+# or an upstream chain of trust task.
+#-----------------------------------------------------------------------------------------------
+# valid_artifact_rules: [{
@JohanLorenzo
JohanLorenzo Nov 2, 2016 Contributor

TIL: JSON is actually a subset of YAML. I wonder if making the configuration purely yaml, is a good idea as a matter of readability. What do you think?

eg:

valid_artifact_rules: 
  - 
    netlocs: 
      - queue.taskcluster.net
    path_regexes: 
      - "^/v1/task/(?P<taskId>[^/]+)(/runs/\\d+)?/artifacts/(?P<filepath>.*)$"
    schemes: 
      - https
@escapewindow
escapewindow Nov 2, 2016 Member

Sure! Done.

@escapewindow escapewindow address review comment: make yaml more readable and less json
1e158be
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling 1e158be on escapewindow:yaml into b401d2f on mozilla-releng:master.

@escapewindow escapewindow update changelog
337c178
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling 337c178 on escapewindow:yaml into b401d2f on mozilla-releng:master.

@escapewindow escapewindow merged commit ccc1b31 into mozilla-releng:master Nov 2, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@escapewindow escapewindow deleted the escapewindow:yaml branch Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment