ci: add group for core docs to pullapprove - #40225

I am a little confused:

• What aboud groups other than core?
• Shouldn't the files be part of one group only? (Currently they are part of both fw-core and docs-core. Is that intentional? How does PullApprove handle that?)

Initially I thought exactly like you, @gkalpak. I thought there'd be one config for comp: docs, but there's actually a number of docs config blocks in this config. You'll find docs-getting-started-and-tutorial, docs-marketing, docs-observables, docs-packaging-and-releasing, docs-cli, docs-libraries, docs-schematics, docs-infra, and this would add docs-core to ensure those paths are appropriately labeled. Each of those groups tackles different blocks of the docs paths for various reasons. Some require different approvers. Some have different labels. So it doesn't quite make sense to put them in one docs block. And we can't leave it in the fw-core by itself and also apply comp: docs, because there's more than just docs there. So it requires a separate docs only group for core content.

Also to answer your question of how does PullApprove handle that, in the case that a code path exists in two groups, both configs apply. So it applies both labels. We have this situation already in a couple of our PullApprove group configs here. We could remove the aio content from fw-core and only include it in docs-core, but then it wouldn't get the comp: core label unless we applied it as well in the docs-core group. I'm not sure if the auto label config section support multiple labels or just one. So I'd have to test that.

Initially I thought exactly like you, @gkalpak. I thought there'd be one config for comp: docs, but there's actually a number of docs config blocks in this config.

Yes, I know that there are multiple blocks containing docs-related files 😁

Currently, we distinguish docs into two main categories: API docs and non-API docs (i.e. guides, tutorials, marketing pages, etc.)
API docs are extracted from JSDoc comments inside the source code files, so the files that affect API docs belong to a specific package (e.g. packages/core/**, packages/forms/**, etc.).
Non-API docs live in aio/content/** and they include content (i.e. HTML or Markdown files, images) and examples (which are mini-apps from which we extract code snippets to show in the docs).

The way we have these different types of docs assigned to groups currently is this:

• The files that contain API docs are essentially the same files that contain the source code for a package, so they are assigned to the corresponding "package" group (for example, fw-core, fw-common, fw-router, etc.).
• The non-API docs that are related to a specific package, are also assigned to the corresponding "package" group. For example a guide on routing is assigned to fw-router, a docs example for forms is assigned to fw-forms, etc.
• The rest of the non-API docs (that are broader and do not correspond to a specific package) are assigned to different subcategories, such as docs-marketing (for marketing pages content), docs-observables (for guides related to observables), etc.

So, what this PR currently does is create a new group (docs-core) for the non-API docs that are related to the core package (or some more generic docs that are also assigned to the core package due to being related to "core" Angular concepts). If anything, I would expect this PR to include similar groups for the non-API docs that are related to the rest of the Angular framework packages, for example:

• fw-forms --> docs-forms
• fw-router --> docs-router
• fw-service-worker --> docs-service-worker
• ...etc.

(That was my first question.)

in the case that a code path exists in two groups, both configs apply.

🎉

I'm not sure if the auto label config section support multiple labels or just one. So I'd have to test that.

I think this is worth investigating. It would be better if we could keep files in one group. For example, if we want to add a new guide in the future, we would only need to add it in one place to get the correct labels. By referencing the files in two groups (e.g. fw-core and docs-core), we would have to remember to add them to (and remove them from) both groups.

To summarize, I think breaking up the "package" groups (core, forms, router, etc.) into separate groups for source code (fw-*) and non-API docs (docs-*) is definitely an improvement over the current situation (but it should be done for all packages to remain consistent).

If possible, it would be preferable imo to only reference files in one group (instead of having them in both fw-* and docs-*). By looking at the docs, this is likely not supported by PullApprove, though 😞

I think this is worth investigating. It would be better if we could keep files in one group. For example, if we want to add a new guide in the future, we would only need to add it in one place to get the correct labels. By referencing the files in two groups (e.g. fw-core and docs-core), we would have to remember to add them to (and remove them from) both groups.

One group is just not possible for fw-core or any group that requires docs. The labeling system for PullApprove applies to the entire group and cannot conditionally apply labels to specific sub paths within that group. fw-core includes paths that are not docs. So in using only one singular group with multiple labels, all code within that group would get both comp: core and comp: docs, which is incorrect. Two groups for each block are required.

To summarize, I think breaking up the "package" groups (core, forms, router, etc.) into separate groups for source code (fw-) and non-API docs (docs-) is definitely an improvement over the current situation (but it should be done for all packages to remain consistent).

Yeah I was looking at that. I'm happy to create subsequent PRs for each of these blocks to ensure we have clear documentation of all of these section changes.

View the results of /pullapprove-test

Verified that PullApprove does not support multiple labels at a time. So separate sections and duplication of paths are unfortunately required. Labels must be a single string.

I just added all the new sections as a separate commit to this PR. This should now cover all docs paths for every component we have.

Possibly simpler alternative approach: #40273
(It remains to be determined if it works as intended.)

Possibly simpler alternative approach: #40273
(It remains to be determined if it works as intended.)

Oh, I hadn't realized there was already work being done here; I was just trying to fix the config for my PR that pullapprove removed comp: docs from.

I don't think reviewers is required - the docs have an example of a group that has no reviewers here
I was testing the config in #40272 and it seems to work nicely as long as we're okay with comp: docs for everything under the guide and examples folders.

I hadn't even thought of that! I'll remove all the reviewers blocks from the new sections. Thanks!

Closing this PR as we're going to rollback auto-labeling for now. I've got a design doc to discuss a better approach.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}