[WIP][QA] Canonical Code Coverage Teams and CODEOWNERS Assignment

[wayneseymour]

Canonical Code Coverage Teams and CODEOWNERS Assignment

WIP - Dumping ground for ideas (stolen from Brian S) :)

Goals

High Level

• Build Code Owners file per ci run
• Assign code coverage teams upon code coverage ingestion
  • Remove the teams assignment pipeline http put
    • This removes the need to to deal with the formatting issue of the painless script

Impl Details

How it works now

Currently, the coverage ingestion process uploads an "inlined" version of the team assignment painless script, via the ES Client.

[spalger]

I'm thinking the config file would be something like this:

interface Team {
  title: string;
  githubGroup: string;
  patterns: string[];
}

export const TEAMS: Team[] = [
  {
    title: 'Operations',
    githubGroup: '@elastic/kibana-operations'
    patterns: [
      '/src/dev/',
      '/src/setup_node_env/',
      '/src/optimize/',
      '/src/es_archiver/',
      '/packages/*eslint*/',
      '/packages/*babel*/',
      '/packages/kbn-dev-utils*/',
      '/packages/kbn-es/',
      '/packages/kbn-optimizer/',
      '/packages/kbn-pm/',
      '/packages/kbn-test/',
      '/packages/kbn-ui-shared-deps/',
      '/src/legacy/server/keystore/',
      '/src/legacy/server/pid/',
      '/src/legacy/server/sass/',
      '/src/legacy/server/utils/',
      '/src/legacy/server/warnings/',
      '/.ci/es-snapshots/',
      '/vars/',
    ]
  },
  ...
]

We could then take the TEAMS export and use that to generate the .github/CODEOWNERS file, as well as the painless script used by the code coverage tooling. We don't need to commit the painless script to the repo AFAIK but we do need to commit the CODEOWNERS file, which means we would need a script that runs the generation logic and then a check in CI that makes sure it's up to date

[wayneseymour]

Ahh, this makes more sense now. I knew you were on to something last week, but now I grok it.
I'll follow up with my boss asap. Thanks man.

[wayneseymour]

@spalger @LeeDr I was interested in what the community has in this space.
I came across this guy: https://www.npmjs.com/package/codeowners

FYI:

I ran two of it's commands, list all and list unowned:
codeowners_audit.log
codeowners_audit_unowned.log

Just an interesting finding.

[LeeDr]

I think the general problem is that we want all code to have a team assignment for code coverage reports but don't necessarily want all those code paths to require team review of PRs (which is what the CODEOWNERS file does).

[wayneseymour]

but don't necessarily want all those code paths to require team review of PRs (which is what the CODEOWNERS file does).

@LeeDr trying to understand this statement. It sounds like you mean we want the assignments, w/o using Codeowners. Is that correct?

[wayneseymour]

Linking the previous discussion: #72150 (comment)

[spalger]

I think the general problem is that we want all code to have a team assignment for code coverage reports but don't necessarily want all those code paths to require team review of PRs (which is what the CODEOWNERS file does).

Sounds like we want to support different pattern list in those cases, patterns would apply to both codeowners and code coverage, but codeownerPatterns could apply to just codeowners and coveragePatterns could just apply to coverage ownership. We own this config format, we can encode whatever information we want into it. It would still be a single source of truth about these things.

[wayneseymour]

We still have at least one outstanding issue.
For the coverage visualizations in Kibana Stats, we show team assignments.
We have verified that we can upload > 1 team assignment per path, but perhaps we should only show one team assignment even though there can be more.

@LeeDr and I think we could just use the first team as the "priority" and only show that one.

Your thoughts? @spalger

[spalger]

So, is there a real use-case for there being more than one owner per file when it comes to coverage?

If not then we could add a check to CI that verifies that every file in the repository maps to exactly one "owner". This check would create a stream of all the files in the repo, and then run them through some sort of matcher to determine which coverage/codeowner patterns match it. If more or less than one code owner matches it could throw an error and prevent changes to the repo until the configuration is updated. This is what we do for typescript projects and it's proven effective. Every typescript file in the repository is validated to match a single project and that ensures that we are always checking types in CI. The same could be true here and would ensure that the coverage config stays up to date as files move around without QA constantly having to update or resync this config with reality.

If we decide that allowing more than 1 team assignment per path is actually useful then I'm not sure what I think we should so. I'd need to see the visualizations, but my instinct is that if we have a file owned by both Team A and Team B, then both teams should get credit for that file. Not just whichever team is listed first in the config. I can imagine an argument for the "first" team approach, but I think that it's very challenging to understand the implications of the ordering in the ownership config and relying on it will likely introduce ambiguity that makes the visualizations less useful/accurate.

Enforcing that a single "owner" is always assigned to every path removes a lot of ambiguity if you ask me.

[wayneseymour]

Yeah that's our line of thinking as well, regarding one owner.
Removing ambiguity seems wise, particularly since neither of the 3 of us can guess up a case.

I must say I like the idea of QA not having to keep the books on the file to team mappings.

The other outstanding issue is that, at this moment, the logic for inlining the painless script is not exposed. I still agree that with the new approach, we would no longer need to store the script, as it's generated, but the issue of uploading it to the cluster still stands.

[spalger]

Well, if we can establish confidence in the assignment logic because it's backed by a CI check that verifies that every file is mapped to a specific team maybe we don't need to use a script anymore and can instead assign ownership when we're ingesting the coverage?

[wayneseymour]

That has my vote, what say you @LeeDr

[LeeDr]

We are assigning ownership during ingest (using an ingest pipeline). But I think you're saying to make the assignments before we ingest it, in the code coverage scripts. I guess I'd be OK with that if it was relatively simple to do.

[LeeDr]

I would like to make an update, and test going through the current process. It's not great.

https://github.com/elastic/kibana/pull/73336/files

[wayneseymour]

Once we have the logic in place to output the two lists (teams, codeowners), it'd just be a lookup.

[elasticmachine]

Pinging @elastic/kibana-qa (Team:QA)

[LeeDr]

We discussed this in a zoom call earlier today and agreed that if multiple teams want the same code paths reflected in their code coverage reports we can support that. The simplest solution then is to always make it match the codeowners relationship.
Since we want all code paths in the code coverage data, and not all to require team review. It seems we need data representing this;

codePath, [teams], review(boolean)

teams is a list of 1 or more teams
review (or codeowners) indicates whether or not it goes into the CODEOWNERS file

One point we discussed on the lookup is that we can't lookup the coveredFilePath from code coverage directly in this data because the whole coveredFilePath won't be there. We could split the coveredFilePath and then try looking up the first 5 levels. If we don't find a match we try with the first 4 levels, and so on until the lookup finds a match.

[wayneseymour]

@LeeDr after pondering this, I'm wondering if it's good to ever NOT define a path in CODEOWNERS.
Particularly since we are planning on a follow-on pr to check that all paths have ownership assigned in CODEOWNERS

[wayneseymour]

@LeeDr I'm currently working out the lookup.
So, I just browsed Kibana Stats and selected the first covered file path.
That paths happens to be packages/kbn-ui-framework/src/components/bar/bar.js.
I dont see that in the original codeowners file, thus it does not exist in the new authoritative list either.
I built the authoritative list from the codeowners file.

I "feel" like some amalgamation of your previous work in the painless script could be of use here.
I'd just have to encode that into the authoritative list.

Your thoughts?

[LeeDr]

Yes. The CODEOWNERS is a subset of the code coverage team assignment list. We want code coverage team assignment for all the product code (not required but OK on test code).
But teams don't want to team approval required on every code path which is what CODEOWNERS does.

[wayneseymour]

@spalger @LeeDr
We need a zoom call to flush out some finer details.

Currently, the source of truth is an array of objects, that gets converted into a Map.
The object array satisfies making it easy to approach, to modify the owners, per path.

Lee would like a structure that essentially mimics the Map object, having all the paths as the keys.
This way could make it easier to defined deep paths for not only codeowners, but also code coverage team assignments.

What time is good gents?

[wayneseymour]

Also, @spalger how do we want to handle committing the generated CODEOWNERS file?

[spalger]

we do need to commit the CODEOWNERS file, which means we would need a script that runs the generation logic and then a check in CI that makes sure it's up to date

[LeeDr]

Notes from discussion today;

[
/*
Team1 
*/
{files: ['/test/functional/apps/somepaths`], excludeFiles: ['...'] ,  approvalRequiredA.K.A.codeOwners:  @elastic/team1 },
files: ['/test/functional/apps/stuff`] , coverageTeam: teamQA,
files: ['/test/functional/apps/stuff`] , coverageTeam: teamQA, approvalRequiredA.K.A.codeOwners: @elastic/team1 ,
excludeFiles: ['...'] 


/*
Team2 
*/
files: ['/test/functional/apps/somepaths`] , coverageTeam: teamQA, approvalRequiredA.K.A.codeOwners: @elastic/team2 ,
files: ['/test/functional/apps/stuff`] , coverageTeam: teamQA,
files: ['/test/functional/apps/stuff`] , coverageTeam: teamQA, approvalRequiredA.K.A.codeOwners: [ @elastic/team1, @elastic/team2] ,

]

[wayneseymour]

Notes from further discussion:

[
  {
    team: 'qa',
    approvers: ['@elastic/kibana-qa'], // Written to CODEOWNERS
    requiresApproval: { // Written to CODEOWNERS
      files: ['/test/functional/apps/**'],
      exclusions: [],
    },
    noApproval: { // Not written to CODEOWNERS
      files: ['/test/functional/apps/**'],
      exclusions: [],
    }
  },
  {
    team: 'app',
    approvers: ['@elastic/app'], 
    requiresApproval: { 
      files: ['/src/app/'],
      exclusions: [],
    },
    noApproval: { 
      files: ['/test/functional/apps/**'],
      exclusions: [],
    }
  },
]

[wayneseymour]

@spalger Any ideas on this ownership config file:

interface OwnerRecord {
  requiresApproval: {
    approver: string, // CODEOWNERS Team Name
    files: string[], // Files listed in CODEOWNERS
  };
  noApproval: {
    team: string;  // Code Coverage Team Name
    files: string[] // Files not listed in CODEOWNERS, but are listed for code coverage team assignment
  },
}

export const ownershipConfig: OwnerRecord[] = [
  {
    requiresApproval: {
      approver: '@elastic/kibana-operations',
      files: [
        '/src/dev/',
        '/src/setup_node_env/',
        '/src/optimize/',
        '/packages/*eslint*/',
        '/packages/*babel*/',
        '/packages/kbn-dev-utils*/',
        '/vars/',
        '!/vars/kibanaCoverage.groovy',
        '!/vars/kibanaTeamAssign.groovy',
      ],
    },
    noApproval: {
      team: 'kibana-operations',
      files: [],
    },
  },
  {
    requiresApproval: {
      approver: '@elastic/kibana-qa',
      files: [
        '/src/dev/code_coverage',
        '/test/functional/services/common',
        '/test/functional/services/lib',
        '/test/functional/services/remote',
        '/vars/kibanaCoverage.groovy',
        '/vars/kibanaTeamAssign.groovy',
      ],
    },
    noApproval: {
      team: 'kibana-operations',
      files: [],
    },
  },
  {
    requiresApproval: {
      approver: '@elastic/kibana-security',
      files: [
        '/src/core/server/csp/',
      ],
    },
    noApproval: {
      team: 'kibana-security',
      files: [],
    },
  },
  {
    requiresApproval: {
      approver: '@elastic/kibana-platform',
      files: [
        '/src/core/server/csp/',
        '/src/core/',
      ],
    },
    noApproval: {
      team: 'kibana-platform',
      files: [],
    },
  },
];

[spalger]

Yeah, it has the same problem we discussed about the explosion of combinations it creates if we add any responsibilities to the config file... What's wrong with the format we decided on in the meeting?

[wayneseymour]

It seems almost identical to me. No?

[wayneseymour]

CI Check must ensure all paths have a code-coverage team assigned.

[wayneseymour]

Remove de-duplication from the source of truth.
That means we'll likely have one stanza per team, that defines owners and one stanza that defines code coverage.

[wayneseymour]

@tylersmalley this issue details what we are attempting in #74691