Allow to provide extra labels

[felixfontein]

Sometimes you don't want to add extra collections to the docs build to resolve some labels, but just provide these label targets. This is useful for community.general which has redirects to infoblox.nios_modules, but we don't want to install that collection to build the docs (it's not under our control and might produce new build errors). Building without that collection gives warnings such as

.../rst/collections/community/general/nios_a_record_module.rst:21: WARNING: undefined label: ansible_collections.infoblox.nios_modules.nios_a_record_module

which would require us to be lenient on docs build errors. With this new option we can add a file (rst/_targets.rst) that contains the given labels.

(Not yet tested.)

[briantist]

This is a good idea, my initial reaction is that I'd maybe like to see this as part of the build.sh.

It dovetails with some of my thoughts about --lenient and --fail-on-errors, in that I'd like to be able to pass them (and maybe other things) to build.sh, optionally, so that sphinx-init isn't the only time when they can be chosen.

So it might fit well with that idea. That would also allow this extra-labels feature to be used when building locally (not using the GH actions).

It wouldn't be too bad to add it only to these actions for now, and do the above later, but would like to see tests added since we're actually testing this action! I realize it's still WIP.

[felixfontein]

I don't think this should be part of build.sh. This is mainly a hack for CI, but never something you should ever do in a real docsite.

[samccann]

I can't argue the merits of one approach or the other but I'm in favor of something that does allow us to inch closer to recommended or mandatory CI tests for all collections in the Ansible package so we can clean up the docs (and links).

[briantist]

We do have tests (for the action, but not the workflow yet), so I would like to see the tests updated to hit this parameter and do at least some naïve checking on the output. If mitigations are added for the malicious input case, we could probably also add a check for that.

[felixfontein]

Hmm, it looks like the tests do not use the actions from this branch, but from the main branch:

Warning: Unexpected input(s) 'provide-links-safely', valid inputs are ['dest-dir', 'collections', 'skip-init', 'fail-on-error', 'lenient', 'antsibull-docs-version', 'provide-link-targets']

(See for example the init step of https://github.com/ansible-community/github-docs-build/runs/6949802920?check_suite_focus=true)

[briantist]

Hmm, it looks like the tests do not use the actions from this branch, but from the main branch:

Warning: Unexpected input(s) 'provide-links-safely', valid inputs are ['dest-dir', 'collections', 'skip-init', 'fail-on-error', 'lenient', 'antsibull-docs-version', 'provide-link-targets']

(See for example the init step of https://github.com/ansible-community/github-docs-build/runs/6949802920?check_suite_focus=true)

hm, that should not be the case, we're using the "local" form where the action is loaded from the files on the runner (so via checkout):

• https://github.com/ansible-community/github-docs-build/runs/6949802920?check_suite_focus=true#step:4:3

And the event is pull_request and not pull_request_target so the default checkout (and the one we're using) is the merge commit:

• https://github.com/ansible-community/github-docs-build/runs/6949802920?check_suite_focus=true#step:2:156

I'll see what I can find, but looking briefly I think it might just be a mismatch on the name of the option provided.

[briantist]

So with the small change I committed they pass, I have a few more suggestions in this batch, and one I can't suggest directly in the UI, so I won't apply them yet.

Will leave it up to you if you want to keep the matrix name mismatched or make it match the param name.

[briantist]

Looks great, thank you for adding the tests!
I'll hold off on merging in case you had any other changes you wanted to make. If not then it's ready to go from my POV, feel free to merge :)

[felixfontein]

I think I'm done (with the first version), so let's merge. I'll probably try this out with community.general, and then I'll see whether something is missing ;-)

[felixfontein]

@briantist thanks a lot for your review!

[briantist]

thanks for contributing @felixfontein !