Closed Bug 1381870 Opened 7 years ago Closed 6 years ago

Move gecko-specific bits of tcadmin into the Gecko tree

Categories

(Taskcluster :: Operations and Service Requests, task)

Product:
Taskcluster
Component:
Operations and Service Requests
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dustin, Assigned: dustin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

bug1381870.patch
4.78 KB, patch
tomprince
: review+
Details | Diff | Splinter Review 
Let's configure all of the roles, hooks, etc., using in-tree code (but not running in automation, since automation does not have the scopes to do so)
The idea is to have some commands like
 ./mach tcadmin check-config
and
 ./mach tcadmin update-config
that would enforce the bits of gecko-specific TC configuration we're currently managing manually.  Roughly, terraform : AWS :: tcadmin : TC, although quite a bit simpler.

The `check-config` command wouldn't require any auth, so I'd like to run that in a cron.yml task, with notifications to taskcluster-notifications.  Then someone with credentials would need to run `update-config` as necessary.

Does that seem reasonable?
Flags: needinfo?(garndt)
(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> Roughly, terraform : AWS :: tcadmin : TC,
> although quite a bit simpler.

Exactly what came to my mind

> 
> The `check-config` command wouldn't require any auth, so I'd like to run
> that in a cron.yml task, with notifications to taskcluster-notifications. 

Is this so that if the config we currently have differs from the one defined in tree, we'll get a notification?


> Does that seem reasonable?

I love having this defined in-tree/code somewhere!
Flags: needinfo?(garndt)
Blocks: 1407681
No longer blocks: 1407681
Depends on: 1437964
I still want to do this, but frankly it's both getting more of my attention than it deserves (there are higher-priority things) and too little to actually complete it.  Perhaps I will revisit if someone else doesn't pick this up.
Assignee: dustin → nobody
I think the work on actions as hooks (bug 1415868) is going to make this pretty painfully necessary.
Assignee: nobody → dustin
So I've talked to Tom about this a few times and I *think* I understand his concerns.. please correct me if I'm wrong!  The main concern is that it ties the status of the global (across all projects) config to a single project (probably mozilla-central).  It's also weird that that project can't *enforce* the global config, but only complain when it's wrong.

Things are about to get worse.  With the landing of actions as hooks, the behavior of in-tree actions is pretty inter-dependent on the detailed configuration of hook resources.  In particular, as implemented the hook configuration depends on `.taskcluster.yml` and on the actions defined in-tree.

So, we have a question of where to put the configuration (details about projects, etc.), and where to put the code that's responsible for making that configuration real.

There's a secondary question, which Jonas has raised, about how to express the necessary Taskcluster resources based on that configuration.  Jonas would like that to be declarative, but I'm not convinced that's possible other than by essentially listing every resource in a YML file and manually ensuring all of the necessary, complex correspondences are in place through the review process.  JSON-e is not nearly expressive enough (it quickly becomes unreadable), and I doubt Terraform could express the dependencies either, without a whole lot of copy pasting. I don't think we understand the problem well enough to design a new DSL for the purpose.

That means that the configuration and the means of turning that into Taskcluster resources will probably not be very loosely coupled.  So they should probably be in the same repository.

The configuration *definitely* should not be in mozilla-central.

So I think the logical conclusion is that we need to design this such that ci-configuration is self-contained: aside from providing data for other tools, it contains enough code to interpret the configuration and create/update/delete Taskcluster resources to match.  Basically tcadmin, but a more comprehensive implementation.

While I embark on that .. are there other practical options here?
Flags: needinfo?(mozilla)
Flags: needinfo?(jopsen)
I think that is a reasonable approximation of my concerns with moving `ci-configuration` + `tc-admin` into mozilla-central.

I'm curious what the pain points for keeping the existing split between ci-configuration and tc-admin are? The only one that I've run into so far, it is not possible to do a trial run against a new version of ci-configuration. I've hacked around that by pointing at a local server, but it should be straigtforward to make the tools optionally take a local path, at least in no-op mode.
Flags: needinfo?(mozilla)
One of them is that tc-admin is a mix of Firefox-specific stuff and Taskcluster-specific stuff.  Also, it's a JS app and the rest of the associated mechanics are in Python.

There is an option of having three repos -- mozilla-central, ci-configuration, and ci-admin; with the latter being a Python version of tc-admin.  But as you've noted it's pretty annoying to try to coordinate changes to ci-configuration and ci-admin, so why not combine them?

It's worth noting that we also want to protect these repos against changes, too -- ideally fewer people than "level 3" can modify either the configuration (data) or the code that implements that configuration.

Putting this all in one repo would work well in the sense that any change that lands would have to go through the usual bug + review process, and then the person landing the patch would be responsible for running the result (and supplying the credentials to do so).
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #8)
> There is an option of having three repos -- mozilla-central,
> ci-configuration, and ci-admin; with the latter being a Python version of
> tc-admin.

I *think* I *slightly* prefer this option, although I'm not sure I can articulate
a good reason for the preference. 

:+1: to switching to python

> But as you've noted it's pretty annoying to try to coordinate
> changes to ci-configuration and ci-admin, so why not combine them?

I noted it in order to downplay. :)

> It's worth noting that we also want to protect these repos against changes,
> too -- ideally fewer people than "level 3" can modify either the
> configuration (data) or the code that implements that configuration.

Yes.
> Jonas would like that to be declarative, but I'm not convinced that's possible other than by essentially listing every resource in a YML

I think that's a fair point. What if use python code to generate the YAML file?
 1) A python script generates a YAML lists all resources, and what namespaces are managed
 2) A different python script takes the declarative output from (1) and creates/updates/deletes resources in TC

That way it's easy to diff the intermediary YAML file and see what the changes are.
It's also easy to inspect the YAML file to see what resources are created?

Note: whether (1) happens with python, json-e, jinja2, or bash-scripting is less important to me.
      As long as the output is easy to read, and the scritps in (1) are pure-functional, ie. they don't call APIs.
Flags: needinfo?(jopsen)
One thing I've been worried about is updating roles like mozilla-group:vpn_sheriffs, as those roles may have other scopes added "manually" that we want to preserve.  I think the way to fix that is to instead manage roles like project:gecko:action-access:vpn_sheriffs and assume:.. that role (manually) in the relevant mozilla-group:.. roles.

Then the input to (2) is basically:

 * a set of "managed" namespaces
 * a set of desired resources

and (2) consists of enumerating the runtime configuration in the managed namespaces and deleting anything unrecognized, together with creating/updating the desired resources.
> I think the way to fix that is to instead manage roles like project:gecko:action-access:vpn_sheriffs and assume:.. that role (manually) in the relevant mozilla-group:.. roles.

This seems sensible.

> Then the input to (2) is basically: [...]

This seems like the declarative intermediate state that Jonas was asking for (whether it needs to be dumped to disk or passed via a library is something that can be resolve later).

> > There is an option of having three repos -- mozilla-central,
> > ci-configuration, and ci-admin; with the latter being a Python version of
> > tc-admin.
> 
> I *think* I *slightly* prefer this option, although I'm not sure I can
> articulate
> a good reason for the preference. 

Thinking on this more, I think it makes sense to keep *some* code that accesses the configuration separate from the configuration, as long as there is any code that access the configuration that is not in the config repo (such as mozilla-taskcluster).
> Thinking on this more, I think it makes sense to keep *some* code that accesses the configuration separate from the configuration, as long as there is any code that access the configuration that is not in the config repo (such as mozilla-taskcluster).

That is, all the code touching the config should either be in the same repo as the config, or none of it.
Depends on: 1463778
Can I get a 30% review on https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/ci-admin?

The README is a good place to start.  Note that I have stuck this in a subdir in the ci-configuration repo for momentary convenience but I intend to land it in build/ci-admin (bug 1463778).

Questions I'm interested in:
 - overall design -- am I don't something fundamentally wrong?
 - complexity -- is this unapproachably complex code?
 - features -- is there some important feature missing (that isn't in the README's TODO)?
 - future -- do you see something we'd want to do soon, that this would not support?

I'm not especially worried about line-by-line Python stuff, but if you feel so inclined please feel free to bring it up.
Flags: needinfo?(mozilla)
Flags: needinfo?(jopsen)
Flags: needinfo?(gps)
This is looking good.

- Given that this is going to be in a separate repo, there should be a way to specify a local repo to look at, for testing (but probably not applying?), as well as looking at the repository-of-record. (Just based on reading the README)
- https://github.com/Tinche/cattrs might be useful for the json conversion
- I would try to avoid using formatted strings for holding data (e.g. https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/ci-admin/ciadmin/current/hooks.py#l18)
- Note that `attrs` has a bunch of options for only using bits of it, and for constructing attributes that aren't exposed in the constructor (for e.g. https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/ci-admin/ciadmin/resources/resources.py#l84)

Once Bug 1463778 is done, I think it would make sense to get phabricator and lando turned on, and use them from the start.
Flags: needinfo?(mozilla)
(In reply to Tom Prince [:tomprince] from comment #15)
> This is looking good.
> 
> - Given that this is going to be in a separate repo, there should be a way
> to specify a local repo to look at, for testing (but probably not
> applying?), as well as looking at the repository-of-record. (Just based on
> reading the README)

This is possible already - I'll add a note to the README.

> - I would try to avoid using formatted strings for holding data (e.g.
> https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> ci-admin/ciadmin/current/hooks.py#l18)

I'm not sure what you mean -- how is that "holding data"?

> - Note that `attrs` has a bunch of options for only using bits of it, and
> for constructing attributes that aren't exposed in the constructor (for e.g.
> https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> ci-admin/ciadmin/resources/resources.py#l84)

I'm confused by this, too -- what attributes would be constructed there?

> Once Bug 1463778 is done, I think it would make sense to get phabricator and
> lando turned on, and use them from the start.

Sigh, I suppose you're right.
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #16)
> > - I would try to avoid using formatted strings for holding data (e.g.
> > https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> > ci-admin/ciadmin/current/hooks.py#l18)
> 
> I'm not sure what you mean -- how is that "holding data"?

You got some structured data (that you are looking for a hook, with a given group-id) and are encoding it in specifically formatted string. I haven't looked at all the involved code, but generally it is better to use structured data rather than passing around formatted strings.
 
> > - Note that `attrs` has a bunch of options for only using bits of it, and
> > for constructing attributes that aren't exposed in the constructor (for e.g.
> > https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-configuration/file/tip/
> > ci-admin/ciadmin/resources/resources.py#l84)
> 
> I'm confused by this, too -- what attributes would be constructed there?

Something vaguely like

def make_managed(managed):
    managed = SortedList()
    for m in managed:
        self.manage(m)

resources = attr.ib(type=SortedKeyList, converter=lambda resources: SortedKeyList(resources, key=lambda r: r.id))
managed = attr.ib(converter=make_managed)

def __attr_post_init__(self):
    self._verify()

(typed directly into bugzilla)
The `id` is a unique identifier for the Taskcluster resource, and we do prefix matching on them.  The prefix matching is always after the "=", but still, I think it's easier to say id.startswith(pattern) than (id.kind == pattern.kind and id.subid.startswith(pattern.subid)) (and also, I'm not sure what to call the subId -- for hooks it's '{hookGroupId}/{hookId}').

Good point on using attrs for Resources, though!  Thanks for clarifying..
I'm far from a domain expert in what is being done here. I like the idea of the project (making it easier to define/update the TC platform level settings required to power Firefox CI). The code all seems pretty reasonable. Thank you for going hard on Python 3 for new projects!

Taking a step backward, it seems like the problem being solved is generic to other TC deployments and a generic tool for configuring platform-level features should be part of the redeployability project. That's *massive* scope bloat from the targeted solution for Firefox implemented here though and I don't want to encourage you to go down that road. But it would help to know if you envision this code evolving to be generic enough to support other projects, whether this code needs to be a bridge until such a generic tool exists, etc. The answer drastically changes how I'd go about reviewing the code.

For example, if you tell me this is meant as a short-term bridge or a project to be used only by TC admins who know what they're doing, then I think the current approach of everything living inline in Python code is very acceptable. But there are a lot of scenarios where I could find myself saying things like "I think things would be clearer if there were a separation between code and data: something like how taskgraph works."

Regardless of how the data is defined, things in files like ciadmin/generate/project_roles.py could definitely use a lot more inline docs. For each resource, I'd like to know why it is present and what its expected use is for. This information is currently scattered across probably dozens of bugs and random IRC conversations and email threads. It would be great if the code capture more of the history for why things are the way they are. It may be excessive, but I think comments for literally every single item (like individual scopes) would be useful. Obviously code speaks for itself and this project is better than "the config lives in the database." So comprehensive docs feel like follow-up fodder after an initial implementation has landed.

Since this is Python 3, the used `from __future__` lines are no-ops and can be removed.

Thank you for using attrs. It's one of my favorite new Python packages. Time will tell if I abandon it in favor of https://docs.python.org/3.7/library/dataclasses.html. Sadly, you probably don't want to make Python 3.7 a requirement to run this code, as 3.7 isn't released yet :)

In a perfect world, I'd like to think this could live inside mozilla-central and we'd just always use the latest code on mozilla-central. But mozilla-central isn't currently very friendly to projects that aren't Firefox proper. So having this live in a separate repo is a reasonable decision.

Given that TC admins will run this code, the security of the code is kinda important. I'd love to see a pip requirements file with pinned hashes so we have integrity checking on downloaded packages. I've recently started using pip-compile (from https://github.com/jazzband/pip-tools) for managing pip requirements files to make things more turnkey.

I hope this is useful. It's possible I missed the boat completely since I'm coming into this without much context for the full scope of the problem space!
Flags: needinfo?(gps)
Thanks!

I expect at some point the generic version of this will be supplied by something like a Terraform plugin, so yes, this is intended to be Firefox-specific (and that's why it's in Python, not JS).

This problem just isn't very declarative, and trying to make it declarative (I have an etherpad a few comments back where I worked on that) results in something that checks the "declarative" box but very emphatically un-checks the "readable" box, to the point where even I would not know how to modify it to support anything new.  So like taskgraph, I've tried to take a hybrid data-and-code approach that I think will produce a more readable, maintainable product.  On Jonas's advice, I've broken the problem into a data-and-code "generation" phase, and a very declarative apply phase (which is just the no-op `ci-admin diff` so far).

I began writing this (months ago) as a mach command in-tree, but it was an awkward fit with the trains: unlike the rest of Firefox code, this code had a "privileged" version which was the one in mozilla-central, and on any later branch (beta, release, ..) was unused and likely different from reality. I'm not 100% convinced the code doesn't belong in ci-configuration, but tomprince's argument -- that if any code is accessing ci-configuration from outside the repo, then *all* code should access it that way -- seemed sensible so I followed it.

I'll pin the versions once it's ready to go.  Thanks for the pip-tools pointer!
I'm not going to use cattr.  First:

>>> import cattr
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/dustin/p/ci-admin/sandbox/lib/python3.5/site-packages/cattr/__init__.py", line 2, in <module>
    from .converters import Converter, UnstructureStrategy
  File "/home/dustin/p/ci-admin/sandbox/lib/python3.5/site-packages/cattr/converters.py", line 2, in <module>
    from typing import (Mapping, Sequence, Optional,
ImportError: cannot import name '_Union'

that doesn't inspire confidence!  But more importantly, it doesn't seem to support renaming properties on structure/unstructure.  I'd like to have that for hook metadata (which is in a `metadata` property in JSON, but should be flat in the Python data structure).  It's possible to fix this with a hook, but that hook's body will be identical to the toJSON/fromJSON functions I already have.
I'm thinking about making a scope-grants.yml in ci-configuration; docs would be something like the below.  Thoughts?  I'm worried about how to express all of this while still taking advantage of parameterized roles.

---

Grants have the general form:

  - grant: <permission(s)>
    to: <subject(s)>

Either can be a list, and will be expanded; that is,

  - grant: [perm1, perm2]
    to: [subj1, subj2]

is equivalent to

  - grant: perm1
    to: subj1
  - grant: perm2
    to: subj1
  - grant: perm1
    to: subj2
  - grant: perm2
    to: subj2

The available permissions are:

  "some-scope"
    -- raw strings are assumed to be scopes

  {trigger-hook: {level: <l>, actionPerm: <ap>}}
    -- trigger the given hook at the given SCM level

  {route: <route>}
    -- queue:route:<route>

  {index: <namespace>}
    -- both queue:route:index.<namespace> and index:insert-task:<namespace>

The available subjects are:

  {mozilla-group: <group>}
    -- users in the Mozilla LDAP group <group> (to work correctly, the role
    `mozilla-group:<group>` must contain scope `assume:project:gecko:group-access:<group>`)
  ## project:gecko:group-access:<group>`

  {project: <proj>, trust_domain: <td>, level: <level>}
    -- all operations on project with alias <proj>.  Use "*" to match all
    projects, in which case '<..>' in the resulting scopes will be
    parameterized to the project alias.  If the trust domain or level are
    omitted, applies to all trust domains or levels.
  ## all of {role_root}:branch:{domain}:level-{level}:<proj>

  {project: <proj>, only: {pushes: true}, trust_domain: <td>, level: <level>}
    -- Like {project: ..}, but only for pushes (not actions or cron tasks)
  ## assume:{role_root}:push:{domain}:level-{level}:{alias}'

  {project: <proj>, only: {action: <action>}, trust_domain: <td>, level: <level>}
    -- Like {project: ..}, but only for the given action (the action name can be
    a * pattern)
  ## ??

  {project: <proj>, only: {cron: <cron>}, trust_domain: <td>, level: <level>}
    -- Like {project: ..}, but only for the given cron task (the cron name can be
    a * pattern, such as nightly-*)
  ## ??

  {level: <l>, trust_domain: <td>}
    -- All projects at level <l>; if trust_domain is specified, applies only
    to projects in that trust domain, otherwise to all
  ## moz-tree:level:<l>:<td>

  {project-feature: <feature>, trust_domain: <td>, level: <l>}
    -- All projects at level <l> and trust_domain <td> with feature <feature>
    If level or trust_domain are omitted, applies to all.  `<..>` in any
    granted scopes will refer to the project alias.
  # {role_root}:feature:{feature}:{domain}:level-{level}:*
I think the ability look at the generated YAML output and diff this, solves most of the issues with the code not being declarative. It would be nice if it was far more declarative, but that might be something we can work towards over time.
While it's possible to use json-e to make it declarative the number of exceptions and special-cases does make this hard.

One possible way to make it more declarative would be to make templates and then have python code to specify the various combinations of parameters. But like I said, that probably a future improvement. Being able to diff the output before applying it seems really nice.

ps. the grants idea looks really neat.
Flags: needinfo?(jopsen)
Good.  I think the grants will be pretty declarative.. I just need to find a way to either support general grants ("all actions matching release-* for all projects matching mozilla-esr*") or make it clear exactly which grants are supported.
Asking for review on a new repo is always awkward, so I'm gong to do this via needinfo.

The repo is
  https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-admin/file

I think I've taken all of the above advice into account.  I've moved a bunch of things out into followup, and I'll start filing those shortly.  They are, roughly (and included here only to save you noticing their absence):

* CI
  * Add .taskcluster.yml
  * Maybe get this visible in Treeherder
  * Run tests + lint on push
  * Run diff on a cron task
* rename project.gecko_repo to something that doesn't sound boolean
* incorporate /projects/nss,nss-try (just grant scopes directly with tc-extra-scopes)
* move lists of scopes for features, branches, etc. out to configuration (see below)
* check project levels
* Name crontasks after aliases, not paths
* Configure all gecko-relevant AWS workerTypes
* Configure queue / worker-manager metadata about workerTypes, including worker actions
* Configure pulse hooks for hg pushes
* rename cron hooks to use alias
* rename project-releng to project-gecko
Flags: needinfo?(mozilla)
Attached patch bug1381870.patch (obsolete) — — Splinter Review 
corresponding ci-configuration patch
Attachment #8982271 - Flags: review?(mozilla)
* CI - https://bugzilla.mozilla.org/show_bug.cgi?id=1465838
* incorporate /projects/nss,nss-try (just grant scopes directly with 
  tc-extra-scopes) -- https://bugzilla.mozilla.org/show_bug.cgi?id=1465841
* move lists of scopes for features, branches, etc. out to configuration (see below)
  -- https://bugzilla.mozilla.org/show_bug.cgi?id=1465842
* check project levels -- https://bugzilla.mozilla.org/show_bug.cgi?id=1465844
* Name crontasks after aliases, not paths -- https://bugzilla.mozilla.org/show_bug.cgi?id=1465846
* Configure all gecko-relevant AWS workerTypes -- https://bugzilla.mozilla.org/show_bug.cgi?id=1465851
  * Configure queue / worker-manager metadata about workerTypes, including worker actions
* Configure pulse hooks for hg pushes -- once we have pulse hooks :)
* rename project-releng to project-gecko -- https://bugzilla.mozilla.org/show_bug.cgi?id=1465853
Here are the changes this will make when run:

$ ci-admin diff --ids-only --ignore-descriptions --ci-configuration-directory=../ci-configuration
- Hook=project-comm/in-tree-action-1-generic
- Hook=project-comm/in-tree-action-1-generic/051fd0e513
+ Hook=project-comm/in-tree-action-1-generic/1f7ad83db7
! Hook=project-comm/in-tree-action-1-generic/22c71b52ec (changed: name, triggerSchema)
! Hook=project-comm/in-tree-action-1-generic/2848139f99 (changed: name, triggerSchema)
! Hook=project-comm/in-tree-action-1-generic/2df6510136 (changed: name, triggerSchema)
! Hook=project-comm/in-tree-action-1-generic/4b6ddb3beb (changed: name, triggerSchema)
- Hook=project-comm/in-tree-action-1-generic/4cded9983c
! Hook=project-comm/in-tree-action-1-generic/60a7e1597b (changed: name, triggerSchema)
- Hook=project-comm/in-tree-action-1-generic/63780ef395
! Hook=project-comm/in-tree-action-1-generic/6422f03f53 (changed: name, triggerSchema)
- Hook=project-comm/in-tree-action-1-generic/6bc0edeb17
! Hook=project-comm/in-tree-action-1-generic/b17ba2d183 (changed: name, triggerSchema)
! Hook=project-comm/in-tree-action-1-generic/c60b2b6f78 (changed: name, triggerSchema)
! Hook=project-comm/in-tree-action-1-generic/da10b47a11 (changed: name, triggerSchema)
! Hook=project-comm/in-tree-action-1-generic/f4c4249360 (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-generic/051fd0e513
+ Hook=project-gecko/in-tree-action-1-generic/1f7ad83db7
! Hook=project-gecko/in-tree-action-1-generic/22c71b52ec (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-generic/2848139f99 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-generic/2df6510136 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-generic/4b6ddb3beb (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-generic/4cded9983c
! Hook=project-gecko/in-tree-action-1-generic/60a7e1597b (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-generic/63780ef395
! Hook=project-gecko/in-tree-action-1-generic/6422f03f53 (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-generic/6bc0edeb17
! Hook=project-gecko/in-tree-action-1-generic/b17ba2d183 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-generic/c60b2b6f78 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-generic/da10b47a11 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-generic/f4c4249360 (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-purge-caches
- Hook=project-gecko/in-tree-action-1-purge-caches/051fd0e513
+ Hook=project-gecko/in-tree-action-1-purge-caches/1f7ad83db7
! Hook=project-gecko/in-tree-action-1-purge-caches/22c71b52ec (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-purge-caches/2848139f99 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-purge-caches/2df6510136 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-purge-caches/4b6ddb3beb (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-purge-caches/4cded9983c
! Hook=project-gecko/in-tree-action-1-purge-caches/60a7e1597b (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-purge-caches/63780ef395
! Hook=project-gecko/in-tree-action-1-purge-caches/6422f03f53 (changed: name, triggerSchema)
- Hook=project-gecko/in-tree-action-1-purge-caches/6bc0edeb17
! Hook=project-gecko/in-tree-action-1-purge-caches/b17ba2d183 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-purge-caches/c60b2b6f78 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-purge-caches/da10b47a11 (changed: name, triggerSchema)
! Hook=project-gecko/in-tree-action-1-purge-caches/f4c4249360 (changed: name, triggerSchema)
! Hook=project-releng/cron-task-comm-central (changed: name)
! Hook=project-releng/cron-task-integration-autoland (changed: name, task)
! Hook=project-releng/cron-task-integration-mozilla-inbound (changed: name, task)
! Hook=project-releng/cron-task-mozilla-central (changed: name, task)
! Hook=project-releng/cron-task-projects-birch (changed: name, task)
! Hook=project-releng/cron-task-projects-cedar (changed: name, task)
! Hook=project-releng/cron-task-projects-elm (changed: name, task)
! Hook=project-releng/cron-task-projects-graphics (changed: name, task)
! Hook=project-releng/cron-task-projects-holly (changed: name, task)
! Hook=project-releng/cron-task-projects-jamun (changed: name, task)
! Hook=project-releng/cron-task-projects-maple (changed: name, task)
! Hook=project-releng/cron-task-projects-oak (changed: name, task)
! Hook=project-releng/cron-task-projects-pine (changed: name, task)
! Hook=project-releng/cron-task-releases-mozilla-beta (changed: name, task)
! Hook=project-releng/cron-task-releases-mozilla-esr60 (changed: name, task)
! Hook=project-releng/cron-task-releases-mozilla-release (changed: name, task)
- Role=hook-id:project-comm/in-tree-action-1-generic
- Role=hook-id:project-comm/in-tree-action-1-generic/2df6510136
- Role=hook-id:project-comm/in-tree-action-1-generic/4b6ddb3beb
- Role=hook-id:project-comm/in-tree-action-1-generic/4cded9983c
- Role=hook-id:project-comm/in-tree-action-1-generic/6bc0edeb17
- Role=hook-id:project-comm/in-tree-action-1-generic/b17ba2d183
- Role=hook-id:project-comm/in-tree-action-1-generic/da10b47a11
- Role=hook-id:project-gecko/in-tree-action-1-generic
- Role=hook-id:project-gecko/in-tree-action-1-generic/2df6510136
- Role=hook-id:project-gecko/in-tree-action-1-generic/4b6ddb3beb
- Role=hook-id:project-gecko/in-tree-action-1-generic/4cded9983c
- Role=hook-id:project-gecko/in-tree-action-1-generic/6bc0edeb17
- Role=hook-id:project-gecko/in-tree-action-1-generic/b17ba2d183
- Role=hook-id:project-gecko/in-tree-action-1-generic/da10b47a11
- Role=hook-id:project-gecko/in-tree-action-1-purge-caches
- Role=hook-id:project-gecko/in-tree-action-1-purge-caches/2df6510136
- Role=hook-id:project-gecko/in-tree-action-1-purge-caches/4b6ddb3beb
- Role=hook-id:project-gecko/in-tree-action-1-purge-caches/6bc0edeb17
- Role=hook-id:project-gecko/in-tree-action-1-purge-caches/b17ba2d183
- Role=hook-id:project-gecko/in-tree-action-1-purge-caches/da10b47a11
- Role=hook-id:project-releng/cron-task-incubator-stylo
- Role=hook-id:project-releng/cron-task-integration-fx-team
- Role=hook-id:project-releng/cron-task-projects-alder
- Role=hook-id:project-releng/cron-task-projects-ash
- Role=hook-id:project-releng/cron-task-projects-cypress
- Role=hook-id:project-releng/cron-task-projects-fig
- Role=hook-id:project-releng/cron-task-projects-gum
- Role=hook-id:project-releng/cron-task-projects-larch
- Role=hook-id:project-releng/cron-task-releases-mozilla-aurora
- Role=hook-id:project-releng/cron-task-releases-mozilla-esr45
- Role=hook-id:project-releng/cron-task-releases-mozilla-esr52
! Role=mozilla-group:active_scm_level_1 (changed: scopes)
- Role=repo:hg.mozilla.org/incubator/stylo-try:cron:nightly-*
- Role=repo:hg.mozilla.org/incubator/stylo:cron:nightly-*
- Role=repo:hg.mozilla.org/projects/alder:cron:nightly-*
- Role=repo:hg.mozilla.org/projects/ash:cron:nightly-*
- Role=repo:hg.mozilla.org/projects/cypress:cron:nightly-*
- Role=repo:hg.mozilla.org/projects/fig:cron:nightly-*
- Role=repo:hg.mozilla.org/projects/gum:cron:nightly-*
- Role=repo:hg.mozilla.org/projects/larch:cron:nightly-*
- Role=repo:hg.mozilla.org/releases/b2g-ota:*
- Role=repo:hg.mozilla.org/releases/b2g-ota:cron:nightly-*
- Role=repo:hg.mozilla.org/releases/mozilla-aurora:*
- Role=repo:hg.mozilla.org/releases/mozilla-aurora:cron:nightly-*
- Role=repo:hg.mozilla.org/releases/mozilla-b2g34_v2_1s:*
- Role=repo:hg.mozilla.org/releases/mozilla-b2g34_v2_1s:cron:nightly-*
- Role=repo:hg.mozilla.org/releases/mozilla-b2g44_v2_5:*
- Role=repo:hg.mozilla.org/releases/mozilla-b2g44_v2_5:cron:nightly-*
- Role=repo:hg.mozilla.org/releases/mozilla-esr45:*
- Role=repo:hg.mozilla.org/releases/mozilla-esr52:cron:nightly-*
- Role=repo:hg.mozilla.org/try:cron:nightly-*

That's:
 * a bunch of minor changes to in-tree actions (tc-admin was a little sloppy about which tc-yaml's it accepted)
 * updates to crontasks (name is just the hook group/id, and the task has a whitespace change)
 * remove unused action hook roles
 * remove unused cron-task roles (for old repos)
 * update scm_level_1's scopes for action hooks
 * remove some unused roles for old repos
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #22)
> I'm thinking about making a scope-grants.yml in ci-configuration; docs would
> be something like the below.  Thoughts?  I'm worried about how to express
> all of this while still taking advantage of parameterized roles.

One initial comment I have on the scope-grant, is that `project` is overloaded. When I was first reading through, I assumed that it referred to the taskcluster concept, but it turns out it was referring to the gecko-taskgraph one. I'm not sure how to resolve that tension.
Attached patch bug1381870.patch — — Splinter Review 
..added level-2 and 3 actions..
Attachment #8982271 - Attachment is obsolete: true
Attachment #8982271 - Flags: review?(mozilla)
Attachment #8982367 - Flags: review?(mozilla)
Blocks: 1431948
Here are my initial notes, on reading the ci-admin code. Things I still need to do:
- compare the code to the tasckluster-admin code for parity
- look at the diff to the current configuration
- look at the tests

(apologies for the poor formatting)

Qs:
- `Resources.__eq__` should this check `.managed`? If not, why not? We should record the reasoning in the code.
  - If it does want to be excluded, just pass `cmp=False` to `attr.ib`.

Nit: 

- hanging indent in setup.py
- ciadmin.diff.textual_diff:
  - missing initial `C` in docstring
  - unified_diff : tofile/fromfile reversed
- Resources.__repr__
  - duplicates ciadmin.resources.util.json_formatter
  - calls .verify and also .to_json which calls .verify

- ciadmin.generate.Projects
  - If .features shouldn't be used, you should make it private (the __init__ argument won't have a leading `_`)

- mixed camelCase and snake_case (at least in ciadmin/generate/in_tree_actions.py)

Followup:
- [IMPORTANT] Move ciconfig out ouf ciadmin.generate and pass an object in, rather than accessing click options a globals.
- factor out applying regexes from cli to resources (`Resources.filter(regex)`)?
- `--color-always`
- can we separate generting the expected configuration from getting the set of namespaces controlled by this.
- moz-tree:level*
- seession global
- I'm slightly unhappy with ciadmin/generate/in_tree_actions.py also querrying the current state, but I don't have a better suggestion
  - Maybe we want to have a separate step that preserves some of the existing config, for this case?
- manual nightly hooks


Defition Concerncs (followup)
- mozilla-group:active_scm_level_3 currently grants (indirectly) `assume:repo:hg.mozilla.org/mozilla-central:cron:nightly-*` (via assume:repo:hg.mozilla.org/mozilla-central:*).
  - If it needs to give a repo role, perhaps `...:branch:default` or `..:branch:*` is more appropriate
  - I think we probably want to get away from giving out any `assume:repo:*` roles, at least for levels > 1 to active_scm_level.

- Is `default` in `repo:...:branch:default` supposed to refer to the mercurial branch name? I don't think anything actually looks at that.
(In reply to Tom Prince [:tomprince] from comment #31)
> - [IMPORTANT] Move ciconfig out ouf ciadmin.generate and pass an object in,
> rather than accessing click options a globals.

I thought this was a rather elegant way to not have to pass options all over the place.  Click doesn't provide a nice "options" object to pass around, either.  Is there a problem with this approach?

> - can we separate generting the expected configuration from getting the set
> of namespaces controlled by this.

Sort of -- we'd still need to fetch a few files to get the list of projects, for example, but could avoid downloading .taskcluster.yml's.  I don't think that's worth the noise it would add to the code.

> - moz-tree:level*

Yup, several open bugs for that already.

> - seession global

Again, why is this problematic?

> - I'm slightly unhappy with ciadmin/generate/in_tree_actions.py also
> querrying the current state, but I don't have a better suggestion
>   - Maybe we want to have a separate step that preserves some of the
> existing config, for this case?

Yeah, the other option is some kind of "don't-delete-matching-things-if" functionality in `ci-admin apply`, and that seemed a lot messier, especially since the "if" in this case is pretty specific.

> - manual nightly hooks

I think the idea is to make these actions, at which time the existing hooks will be deleted.

> Defition Concerncs (followup)
> - mozilla-group:active_scm_level_3 currently grants (indirectly)
> `assume:repo:hg.mozilla.org/mozilla-central:cron:nightly-*` (via
> assume:repo:hg.mozilla.org/mozilla-central:*).
>   - If it needs to give a repo role, perhaps `...:branch:default` or
> `..:branch:*` is more appropriate
>   - I think we probably want to get away from giving out any `assume:repo:*`
> roles, at least for levels > 1 to active_scm_level.

That's kind of the whole security effort driving actions-as-hooks.  So I want to get this landed, then work on actions-as-hooks, and that will let us clean up a lot of these.  There are a few more we can clean up in parallel, such as bug 1431948.

> - Is `default` in `repo:...:branch:default` supposed to refer to the
> mercurial branch name? I don't think anything actually looks at that.

It is - it's parallel to the github repo structure.
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #32)
> (In reply to Tom Prince [:tomprince] from comment #31)
> > - [IMPORTANT] Move ciconfig out ouf ciadmin.generate and pass an object in,
> > rather than accessing click options a globals.
>e
> I thought this was a rather elegant way to not have to pass options all over
> the place.

I am always leery of global variables. I generally prefer just passing arguments to functions.
In this particular case, using globals means that the generation code implicitly depends on having
a click context, and that tests need to monkey patch functions, instead of just passing a structured
object in.

> Click doesn't provide a nice "options" object to pass around,
> either.

I was actually think of having a function that read and parsed all the files from
ci-config up-front, and just pass that object around.

 
> > - I'm slightly unhappy with ciadmin/generate/in_tree_actions.py also
> > querrying the current state, but I don't have a better suggestion
> >   - Maybe we want to have a separate step that preserves some of the
> > existing config, for this case?
> 
> Yeah, the other option is some kind of "don't-delete-matching-things-if"
> functionality in `ci-admin apply`, and that seemed a lot messier, especially
> since the "if" in this case is pretty specific.

The condition in the case actually seems generic for hooks? It seems like it would
be reasonable to add some metadata to the list of managed resources, to indicate that
hooks should only be deleted after they expired. 

This would make it easy to have an option for showing what the configuration would be *now*,
based on a blank slate, which I think might be useful. But, in any case, as I said originally,
this can be a (low-priortiy) follow up.


> > - Is `default` in `repo:...:branch:default` supposed to refer to the
> > mercurial branch name? I don't think anything actually looks at that.
> 
> It is - it's parallel to the github repo structure.

I think (as part of the clean up of roles after this lands), we should consider dropping it, or actually implementing it. I've been pushing to relbranches recently, and noticed that I was still getting `:default` in the scopes.
I've completed my review (although I ended ups skipping the tests).

There are just a couple of more things to consider for followup:
- check-repo-levels from taskcluster-admin probably belongs here too
- a way to interact with a pager and/or force color
Flags: needinfo?(mozilla)
Thanks for the detailed review!  I'll update and re-submit.  I see your point about the global variables.  Sometimes Python's magic isn't great.  The design I've invented looks a lot like Flask, and I've noticed that while Flask apps are succinct, they are *very* difficult to read for a newcomer due to all the globals, magic, and decorators.

(In reply to Tom Prince [:tomprince] from comment #34)
> - check-repo-levels from taskcluster-admin probably belongs here too

Bug 1465844

> - a way to interact with a pager and/or force color

Bug 1467511

> I think (as part of the clean up of roles after this lands), we should consider dropping it, or actually implementing it. 
> I've been pushing to relbranches recently, and noticed that I was still getting `:default` in the scopes.

This will probably make more sense when (if!) we have tasks triggered from pulse messages.  To be honest, I didn't realize we used in-repo branches at all!
Ok, I followed on on the nits/Q's.  The rest can be handled in subsequent bugs.  I've landed this and will now start deploying it in batches.
Things got hung up on
  https://github.com/taskcluster/taskcluster-hooks/pull/104

I'll continue the rollout once that lands.
Comment on attachment 8982367 [details] [diff] [review]
bug1381870.patch

Review of attachment 8982367 [details] [diff] [review]:
-----------------------------------------------------------------

::: actions.yml
@@ +43,5 @@
> +
> +- trust_domain: gecko
> +  level: 1
> +  action_perm: purge-caches
> +  groups: [taskcluster, vpn_sheriff]

releng and/or ciduty maybe?

::: cron-task-template.yml
@@ +29,5 @@
> +scopes:
> +  - assume:hook-id:${hookGroupId}/${hookId}
> +payload:
> +    # this should be a fairly recent decision task image
> +    image: 'taskcluster/decision:2.0.0@sha256:4039fd878e5700b326d4a636e28c595c053fbcb53909c1db84ad1f513cf644ef'

We should follow up to update this to 2.1.0
Attachment #8982367 - Flags: review?(mozilla) → review+
Agreed on both points (as followups) :)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: Operations → Operations and Service Requests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: