[owners] Transition from YAML as owners file format about amp-github-apps HOT 19 CLOSED

I believe the protobuf syntax requires using the key for each instance of a repeated field. For example, this amp-bind validator file shows how repeated fields like tags and attrs are used.

from amp-github-apps.

JSON is hard for humans to write and read... How about protoascii? :D

from amp-github-apps.

That seems like a good option, since contributors will likely be familiar with it anyway

from amp-github-apps.

protoascii sounds good to me!

JSON also has the issue that it doesn't allow comments, which are sometimes useful in the files.

from amp-github-apps.

After chatting offline with @rcebulko, I too am in favor of protoascii.

from amp-github-apps.

Example: current root OWNERS.yaml

# For an explanation of the OWNERS.yaml rules and syntax, see:
# https://github.com/ampproject/amp-github-apps/blob/master/owners/OWNERS.example.yaml

- "?cramforce"
- "?dvoytenko"
- "?jridgewell"
- "**/validator-*.{protoascii,html,out}": "#ampproject/wg-caching"
- "*.md": ampproject/wg-outreach
- "{.*,{{babel,bundles}.config,gulpfile}.js,renovate.json}": ampproject/wg-infra
- "{package.json,yarn.lock}":
  - ampproject/wg-infra
  - ampproject/wg-runtime
  - ampproject/wg-performance

# TODO(#24685): Remove this once most owners files have been update and/or the
# Travis check is implemented.
- "**/OWNERS.yaml": "#rcebulko"

// For an explanation of the OWNERS.yaml rules and syntax, see:
// https://github.com/ampproject/amp-github-apps/blob/master/owners/OWNERS.example.yaml

rules: {
  owners: {
    name: "cramforce"
    notify: NEVER
  }
  owners: {
    name: "dvoytenko"
    notify: NEVER
  }
  owners: {
    name: "jridgewell"
    notify: NEVER
  }
}

rules: {
  pattern: "**/validator-*.{protoascii,html,out}"
  owners: {
    // The validator team should be made aware of all changes to validator files.
    name: "ampproject/wg-caching"
    notify: ALWAYS
  }
}

rules: {
  pattern: "*.md"
  owners: { name: "ampproject/wg-outreach" }
}

rules: {
  pattern: "{.*,{{babel,bundles}.config,gulpfile}.js,renovate.json}"
  owners: { name: "ampproject/wg-infra" }
}

rules: {
  pattern: "{package.json,yarn.lock}"
  owners: { name: "ampproject/wg-infra" }
  owners: { name: "ampproject/wg-runtime" }
  owners: { name: "ampproject/wg-performance" }
}

// TODO(#24685): Remove this once most owners files have been update and/or the
// Travis check is implemented.
rules: {
  pattern: "**/OWNERS.yaml"
  owners: {
    name: "rcebulko"
    notify: ALWAYS
  }
}

from amp-github-apps.

The protobuf message definitions would look like:

enum NotificationControl {
  NONE = 0;
  NEVER = 1;
  ALWAYS = 2;
}

message Owner {
  // Case-insensitive team name or username
  // ex. `ampproject/wg-infra`, `rcebulko`
  required string name = 1;
  optional NotificationControl notify = 2;
}

message Rule {
  repeated Owner owners = 1;
  optional string pattern = 2;
}

message OwnersFile {
  repeated Rule rules = 1;
}

from amp-github-apps.

Thanks for the clarifying example. Does the protoascii format allow for all the separate sections to be combined into one top-level rules section containing multiple owners (+ pattern + notify) sections?

from amp-github-apps.

Which, while protoascii is the most explicit/well-defined syntax, it's also the most verbose

from amp-github-apps.

For comparison, the JSON format (fudging the standard a bit to be more JavaScript-like, supporting comments, non-quoted keys, and trailing commas) would look like:

// For an explanation of the OWNERS.yaml rules and syntax, see:
// https://github.com/ampproject/amp-github-apps/blob/master/owners/OWNERS.example.yaml

{
  rules: [
    {
      owners: [{
        name: "cramforce",
        requestReviews: false,
      }, {
        name: "dvoytenko",
        requestReviews: false,
      }, {
        name: "jridgewell",
        requestReviews: false,
      }]
    },

    {
      pattern: "**/validator-*.{protoascii,html,out}",
      owners: [{
        // The validator team should be made aware of all changes to validator files.
        name: "ampproject/wg-caching",
        notify: true,
      }]
    }, 
    
    {
      pattern: "*.md",
      owners: [{ name: "ampproject/wg-outreach" }],
    },

    {
      pattern: "{.*,{{babel,bundles}.config,gulpfile}.js,renovate.json}",
      owners: [{ name: "ampproject/wg-infra" }],
    },

    {
      pattern: "{package.json,yarn.lock}",
      owners: [
        { name: "ampproject/wg-infra" },
        { name: "ampproject/wg-runtime" },
        { name: "ampproject/wg-performance" },
      ],
    }

    {
      // TODO("24685): Remove this once most owners files have been update and/or the
      // Travis check is implemented.
      pattern: "**/OWNERS.yaml",
      owners: [{
        name: "rcebulko",
        notify: true,
      }],
    },
  ]
}

from amp-github-apps.

@rsimha @danielrozenberg
Update: looks like there is no existing support for parsing the text format of protocal buffers in JavaScript (only the wire format byte array which is, ah, less than human-readable). So, now leaning back toward JavaScript rather than relying on something like Python as a go-between just to parse protoascii/pbtxt files

from amp-github-apps.

@mrjoro please confirm the JSON approach looks good to you if you have a moment

from amp-github-apps.

My main concern is that this format is JSON-like, but isn't actual JSON right (for the reasons you mentioned). I suppose we could just call it a JS format instead of JSON format and be good?

from amp-github-apps.

I think what we're really looking for is JSON5, which is a superset of JSON designed to be more human-readable and -writable. Notably, it supports comments, unquoted keys, single-quotes, and trailing commas.

from amp-github-apps.

For that reason, I'm curious @mrjoro @rsimha if you think we should have our owners files be:

• OWNERS.js since that will trigger correct syntax highlighting in editors (but maybe that would get built/trigger Travis or other side-effects since it's *.js?
• OWNERS.json since that's approximately correct and familiar, but may result in incorrect syntax highlighting
• OWNERS.json5 which is technically accurate but would result in no syntax highlighting at all
• OWNERS without any file extension so that the extension/syntax highlighting can't introduce confusion and developers will work off of existing correct example files

I'd lean toward the first or last options.

from amp-github-apps.

OK, I think that's fine then. :) I have no strong opinion on the filename, so I'll leave that up to y'all.

from amp-github-apps.

The last file name of just OWNERS will give us maximum flexibility, while preventing the need for file extension based exceptions to be made across the build system.

For syntax highlighting, we could lean on this potentially upcoming feature: editorconfig/editorconfig-vote#10

from amp-github-apps.

Conversion script: https://gist.github.com/rcebulko/1bdc4086f8bf3e97a3119d0d9b7cf95e

from amp-github-apps.

[accidentally closed]

from amp-github-apps.