Skip to content

Blurb: validate GitHub issue number #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hugovk opened this issue May 19, 2023 · 8 comments · Fixed by #519
Closed

Blurb: validate GitHub issue number #504

hugovk opened this issue May 19, 2023 · 8 comments · Fixed by #519

Comments

@hugovk
Copy link
Member

hugovk commented May 19, 2023

The short story

It would be nice if we checked GitHub issue numbers are valid.

Long version

Thanks for blurb!

If you accidentally use an (old) GitHub PR number instead of the GitHub issue number, the CI docs build will fail:

Building NEWS from Misc/NEWS.d with blurb
PATH=./venv/bin:$PATH sphinx-build -b html -d build/doctrees  -j auto -q -W --keep-going . build/html 
Error: ../build/NEWS:7[15](https://github.com/python/cpython/actions/runs/5021764661/jobs/9004477237#step:7:16): ERROR: The GitHub ID '13[17](https://github.com/python/cpython/actions/runs/5021764661/jobs/9004477237#step:7:18)2' seems too low -- use :issue:`...` for BPO IDs
make: *** [Makefile:50: build] Error 1

Re:

This is validated in https://github.com/python/cpython/blob/616fcad6e2e10b0d0252e7f3688e61c468c54e6e/Doc/tools/extensions/pyspecific.py#L69-L79

Shall we add some validation to Blurb, to fail sooner?

We could also allow old BPO issue numbers similar to https://github.com/python/cpython/blob/616fcad6e2e10b0d0252e7f3688e61c468c54e6e/Doc/tools/extensions/pyspecific.py#L54-L63, but for new NEWS files created today, should we require only the corresponding GitHub issue number?

@terryjreedy
Copy link
Member

Blurb twice says it wants a gh # and an issue #. Would be nice if it could enforce both, at least when online.

# Please enter the relevant GitHub issue number here:
#
.. gh-issue: 

'here' should be changed to 'below'.

@menkotoglou
Copy link
Contributor

I can work on this.

@menkotoglou
Copy link
Contributor

Getting back to this @hugovk, after some research seems like we can follow two different directions here.

The validation here, doesn't really resolve the case that the issue doesn't exist. This only checks if the issue, is less than the MINIMUM_ISSUE_NUMBER.

We could the following:

  1. If we want to keep the validation only in the blurb module, we could just add requests to blurb and do requests.get("https://github.com/python/<repository>/issues/<issue_number>")

  2. We could add the check to blurb_it and add a GET API call, when calling the relevant endpoint.

I tend to like the first solution better, since it's getting to the root, which is blurb module.

@hugovk
Copy link
Member Author

hugovk commented Jul 22, 2023

Thanks for checking.

Let's avoid network calls, because someone could be working offline.

Instead, let's only check number is in the valid range.

To begin with, we only had PRs on GitHub. We had another tracker for issues. So 1-32425 are only PRs.

We have both issues and PRs for >= 32426. So PRs can be >= 32426, but an issue number must be >= 32426.

So let's validate to make sure the number is >= 32426, that's good enough. It helps avoid using old PR numbers as issue numbers, that's the case we're mainly interested in here.

@menkotoglou
Copy link
Contributor

Thanks @hugovk, I'll work on the fix.

@hugovk
Copy link
Member Author

hugovk commented Nov 23, 2023

@menkotoglou Hi! Any luck on this? Do you need a hand with anything?

@menkotoglou
Copy link
Contributor

Hey @hugovk, thank you very much for the reminder. Some personal issues didn't let me look into this. I'll be spending some time on this soon though.

@hugovk
Copy link
Member Author

hugovk commented Nov 24, 2023

Thanks, no rush though, and just ask if you've any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants