mirror of
https://github.com/saltstack/salt.git
synced 2025-04-15 09:10:20 +00:00
Annotate the sources instead of commenting on the PR
Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
This commit is contained in:
Pedro Algarvio
Pedro Algarvio
parent
a1f8fe74f1
commit
4aad0d2e37
5 changed files with 126 additions and 205 deletions
21
.github/workflows/ci.yml
vendored
21
.github/workflows/ci.yml
vendored
|
|
@ -21,7 +21,8 @@ env:
|
|||
COLUMNS: 160
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
contents: read # for dorny/paths-filter to fetch a list of changed files
|
||||
pull-requests: read # for dorny/paths-filter to read pull requests
|
||||
|
||||
concurrency:
|
||||
# Concurrency is defined in a way that concurrent builds against branches do
|
||||
|
|
@ -35,9 +36,6 @@ jobs:
|
|||
prepare-ci:
|
||||
name: Prepare CI
|
||||
runs-on: ubuntu-latest
|
||||
permissions:
|
||||
contents: read # for dorny/paths-filter to fetch a list of changed files
|
||||
pull-requests: read # for dorny/paths-filter to read pull requests
|
||||
outputs:
|
||||
jobs: ${{ steps.process-changed-files.outputs.jobs }}
|
||||
changed-files: ${{ steps.process-changed-files.outputs.changed-files }}
|
||||
|
|
@ -200,20 +198,6 @@ jobs:
|
|||
with:
|
||||
changed-files: ${{ needs.prepare-ci.outputs.changed-files }}
|
||||
|
||||
pr-checks:
|
||||
name: PR Checks
|
||||
if: ${{ fromJSON(needs.prepare-ci.outputs.jobs)['github-hosted-runners'] && github.event_name == 'pull_request' }}
|
||||
permissions:
|
||||
contents: read
|
||||
pull-requests: write
|
||||
uses: ./.github/workflows/pr-checks-action.yml
|
||||
needs:
|
||||
- prepare-ci
|
||||
with:
|
||||
changed-files: ${{ needs.prepare-ci.outputs.changed-files }}
|
||||
secrets:
|
||||
github-token: ${{ secrets.GITHUB_TOKEN }}
|
||||
|
||||
windows-2016:
|
||||
name: Windows 2016
|
||||
if: ${{ fromJSON(needs.prepare-ci.outputs.jobs)['self-hosted-runners'] }}
|
||||
|
|
@ -479,7 +463,6 @@ jobs:
|
|||
- docs
|
||||
- lint
|
||||
- twine-check
|
||||
- pr-checks
|
||||
- almalinux-8
|
||||
- almalinux-9
|
||||
- amazonlinux-2
|
||||
|
|
|
|||
62
.github/workflows/pr-checks-action.yml
vendored
62
.github/workflows/pr-checks-action.yml
vendored
|
|
@ -1,62 +0,0 @@
|
|||
name: PR Checks
|
||||
|
||||
on:
|
||||
workflow_call:
|
||||
inputs:
|
||||
changed-files:
|
||||
required: true
|
||||
type: string
|
||||
description: JSON string containing information about changed files
|
||||
secrets:
|
||||
github-token:
|
||||
description: The GitHub token to use to comment on PRs
|
||||
required: true
|
||||
|
||||
permissions:
|
||||
contents: read
|
||||
pull-requests: write
|
||||
|
||||
jobs:
|
||||
|
||||
Check-Changed-Files-Docstrings:
|
||||
name: Check Docstrings For Changed Files On PR
|
||||
runs-on: ubuntu-latest
|
||||
if: ${{ github.event_name == 'pull_request' && fromJSON(inputs.changed-files)['salt'] }}
|
||||
|
||||
steps:
|
||||
- uses: actions/checkout@v3
|
||||
- uses: actions/setup-python@v4
|
||||
with:
|
||||
python-version: '3.9'
|
||||
|
||||
- name: Install Dependencies
|
||||
env:
|
||||
PIP_EXTRA_INDEX_URL: https://pypi-proxy.saltstack.net/root/local/+simple/
|
||||
run: |
|
||||
python -m pip install --upgrade pip
|
||||
pip install pre-commit pygithub
|
||||
|
||||
- name: Install Pre-Commit Hooks
|
||||
run: |
|
||||
pre-commit install --install-hooks
|
||||
|
||||
- name: Check Docstrings
|
||||
id: check-known-missing-docstrings
|
||||
continue-on-error: true
|
||||
shell: bash
|
||||
run: |
|
||||
set -o pipefail
|
||||
pre-commit run --hook-stage manual check-known-missing-docstrings \
|
||||
--show-diff-on-failure --color=never \
|
||||
--files ${{ join(fromJSON(inputs.changed-files)['salt_files'], ' ') }} | tee output.txt
|
||||
|
||||
- name: Comment on PR
|
||||
# Comment on PRs if pre-commit triggered a failure
|
||||
if: steps.check-known-missing-docstrings.outcome == 'failure'
|
||||
env:
|
||||
GITHUB_TOKEN: ${{ secrets.github-token }}
|
||||
run: |
|
||||
python .github/workflows/scripts/pr-docstring-comments.py \
|
||||
--org ${{ github.repository_owner }} \
|
||||
--repo ${{ github.event.repository.name }} \
|
||||
--issue ${{ github.event.number }} output.txt
|
||||
1
.github/workflows/pre-commit-action.yml
vendored
1
.github/workflows/pre-commit-action.yml
vendored
|
|
@ -47,6 +47,7 @@ jobs:
|
|||
if: github.event_name == 'pull_request' && fromJSON(inputs.changed-files)['repo']
|
||||
env:
|
||||
SKIP: lint-salt,lint-tests
|
||||
GH_ACTIONS_ANNOTATE: "1"
|
||||
run: |
|
||||
pre-commit run --show-diff-on-failure --color=always --files ${{ join(fromJSON(inputs.changed-files)['repo_files'], ' ') }}
|
||||
|
||||
|
|
|
|||
124
.github/workflows/scripts/pr-docstring-comments.py
vendored
124
.github/workflows/scripts/pr-docstring-comments.py
vendored
|
|
@ -1,124 +0,0 @@
|
|||
import argparse
|
||||
import os
|
||||
import pathlib
|
||||
import sys
|
||||
|
||||
import github
|
||||
from github.GithubException import GithubException
|
||||
|
||||
COMMENT_HEADER = "### Hi! I'm your friendly PR bot!"
|
||||
COMMENT_TEMPLATE = """\
|
||||
{comment_header}
|
||||
|
||||
You might be wondering what I'm doing commenting here on your PR.
|
||||
|
||||
**Yes, as a matter of fact, I am...**
|
||||
|
||||
I'm just here to help us improve the documentation. I can't respond to
|
||||
questions or anything, but what I *can* do, I do well!
|
||||
|
||||
**Okay... so what do you do?**
|
||||
|
||||
I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
|
||||
When I was created we had a *lot* of these. The documentation for these
|
||||
modules need some love and attention to make Salt better for our users.
|
||||
|
||||
**So what does that have to do with my PR?**
|
||||
|
||||
I noticed that in this PR there are some files changed that have some of these
|
||||
issues. So I'm leaving this comment to let you know your options.
|
||||
|
||||
**Okay, what are they?**
|
||||
|
||||
Well, my favorite, is that since you were making changes here I'm hoping that
|
||||
you would be the most familiar with this module and be able to add some other
|
||||
examples or fix any of the reported issues.
|
||||
|
||||
**If I can, then what?**
|
||||
|
||||
Well, you can either add them to this PR or add them to another PR. Either way is fine!
|
||||
|
||||
**Well... what if I can't, or don't want to?**
|
||||
|
||||
That's also fine! We appreciate *all* contributions to the Salt Project. If you
|
||||
can't add those other examples, either because you're too busy, or unfamiliar,
|
||||
or you just aren't interested, we still appreciate the contributions that
|
||||
you've made already.
|
||||
|
||||
Whatever approach you decide to take, just drop a comment here letting us know!
|
||||
|
||||
<details>
|
||||
<summary>Detected Issues (click me)</summary>
|
||||
<pre>{issues_output}</pre>
|
||||
</details>
|
||||
|
||||
---
|
||||
|
||||
Thanks again!
|
||||
"""
|
||||
|
||||
|
||||
def get_previous_comments(pr):
|
||||
for comment in pr.get_issue_comments():
|
||||
if comment.user.login != "github-actions[bot]":
|
||||
# Not a comment made by this bot
|
||||
continue
|
||||
if not comment.body.startswith(COMMENT_HEADER):
|
||||
# This comment does not start with our header
|
||||
continue
|
||||
yield comment
|
||||
|
||||
|
||||
def comment_on_pr(options, issues_output):
|
||||
gh = github.Github(os.environ["GITHUB_TOKEN"])
|
||||
org = gh.get_organization(options.org)
|
||||
print(f"Loaded Organization: {org.login}", file=sys.stderr, flush=True)
|
||||
repo = org.get_repo(options.repo)
|
||||
print(f"Loaded Repository: {repo.full_name}", file=sys.stderr, flush=True)
|
||||
pr = repo.get_pull(options.issue)
|
||||
print(f"Loaded PR: {pr}", file=sys.stderr, flush=True)
|
||||
comment = pr.create_issue_comment(
|
||||
COMMENT_TEMPLATE.format(
|
||||
comment_header=COMMENT_HEADER, issues_output=issues_output
|
||||
)
|
||||
)
|
||||
new_comment_content = COMMENT_TEMPLATE.format(
|
||||
comment_header=COMMENT_HEADER, issues_output=issues_output
|
||||
)
|
||||
for comment in get_previous_comments(pr):
|
||||
if comment.body.strip() != new_comment_content.strip():
|
||||
# The content has changed.
|
||||
print(f"Deleting previous comment {comment}")
|
||||
comment.delete()
|
||||
|
||||
comment = pr.create_issue_comment(new_comment_content)
|
||||
print(f"Created Comment: {comment}")
|
||||
|
||||
|
||||
def main():
|
||||
parser = argparse.ArgumentParser()
|
||||
parser.add_argument("--org", required=True, help="The Github Organization")
|
||||
parser.add_argument("--repo", required=True, help="The Organization Repository")
|
||||
parser.add_argument("--issue", required=True, type=int, help="The issue number")
|
||||
parser.add_argument(
|
||||
"issues_output_path", metavar="ISSUES_OUTPUT_PATH", type=pathlib.Path
|
||||
)
|
||||
|
||||
if not os.environ.get("GITHUB_TOKEN"):
|
||||
parser.exit(status=1, message="GITHUB_TOKEN environment variable not set")
|
||||
|
||||
options = parser.parse_args()
|
||||
if not options.issues_output_path.is_file():
|
||||
parser.exit(1, message=f"The path {options.issues_output_path} is not a file")
|
||||
issues_output = options.issues_output_path.read_text().strip()
|
||||
if not issues_output:
|
||||
parser.exit(1, message=f"The file {options.issues_output_path} is empty")
|
||||
try:
|
||||
comment_on_pr(options, issues_output)
|
||||
parser.exit(0)
|
||||
except GithubException as exc:
|
||||
parser.exit(1, message=str(exc))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
|
|
@ -4,10 +4,14 @@
|
|||
|
||||
Docstrings related tasks
|
||||
"""
|
||||
# pylint: disable=resource-leakage
|
||||
|
||||
import ast
|
||||
import os
|
||||
import pathlib
|
||||
import re
|
||||
import sys
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from invoke import task # pylint: disable=3rd-party-module-not-gated
|
||||
|
||||
|
|
@ -782,6 +786,72 @@ MISSING_EXAMPLES = {
|
|||
],
|
||||
}
|
||||
|
||||
SUMMARY = """\
|
||||
### Hi! I'm your friendly PR bot!
|
||||
|
||||
You might be wondering what I'm doing commenting here on your PR.
|
||||
|
||||
**Yes, as a matter of fact, I am...**
|
||||
|
||||
I'm just here to help us improve the documentation. I can't respond to
|
||||
questions or anything, but what I *can* do, I do well!
|
||||
|
||||
**Okay... so what do you do?**
|
||||
|
||||
I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
|
||||
When I was created we had a *lot* of these. The documentation for these
|
||||
modules need some love and attention to make Salt better for our users.
|
||||
|
||||
**So what does that have to do with my PR?**
|
||||
|
||||
I noticed that in this PR there are some files changed that have some of these
|
||||
issues. So I'm leaving this comment to let you know your options.
|
||||
|
||||
**Okay, what are they?**
|
||||
|
||||
Well, my favorite, is that since you were making changes here I'm hoping that
|
||||
you would be the most familiar with this module and be able to add some other
|
||||
examples or fix any of the reported issues.
|
||||
|
||||
**If I can, then what?**
|
||||
|
||||
Well, you can either add them to this PR or add them to another PR. Either way is fine!
|
||||
|
||||
**Well... what if I can't, or don't want to?**
|
||||
|
||||
That's also fine! We appreciate *all* contributions to the Salt Project. If you
|
||||
can't add those other examples, either because you're too busy, or unfamiliar,
|
||||
or you just aren't interested, we still appreciate the contributions that
|
||||
you've made already.
|
||||
|
||||
Whatever approach you decide to take, just drop a comment in the PR letting us know!
|
||||
"""
|
||||
|
||||
|
||||
def annotate(kind: str, fpath: str, start_lineno: int, end_lineno: int, message: str):
|
||||
if kind not in ("warning", "error"):
|
||||
raise RuntimeError("The annotation kind can only be one of 'warning', 'error'.")
|
||||
if os.environ.get("GH_ACTIONS_ANNOTATE") is None:
|
||||
return
|
||||
|
||||
github_output = os.environ.get("GITHUB_OUTPUT")
|
||||
if github_output is None:
|
||||
utils.warn("The 'GITHUB_OUTPUT' variable is not set. Not adding annotations.")
|
||||
return
|
||||
|
||||
if TYPE_CHECKING:
|
||||
assert github_output is not None
|
||||
|
||||
message = (
|
||||
message.rstrip().replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
|
||||
)
|
||||
# Print it to stdout so that the GitHub runner pick's it up and adds the annotation
|
||||
print(
|
||||
f"::{kind} file={fpath},line={start_lineno},endLine={end_lineno}::{message}",
|
||||
file=sys.stdout,
|
||||
flush=True,
|
||||
)
|
||||
|
||||
|
||||
@task(iterable=["files"], positional=["files"])
|
||||
def check(ctx, files, check_proper_formatting=False, error_on_known_failures=False):
|
||||
|
|
@ -839,6 +909,13 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
path.relative_to(CODE_DIR),
|
||||
*error,
|
||||
)
|
||||
annotate(
|
||||
"error",
|
||||
path.relative_to(CODE_DIR),
|
||||
funcdef.lineno,
|
||||
funcdef.body[0].lineno,
|
||||
"Version {1:r!} is not valid for {0!r}".format(*error),
|
||||
)
|
||||
|
||||
if not str(path).startswith(SALT_INTERNAL_LOADERS_PATHS):
|
||||
# No further docstrings checks are needed
|
||||
|
|
@ -863,6 +940,13 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
funcname,
|
||||
relpath,
|
||||
)
|
||||
annotate(
|
||||
"warning",
|
||||
path.relative_to(CODE_DIR),
|
||||
funcdef.lineno,
|
||||
funcdef.body[0].lineno,
|
||||
"Missing docstring",
|
||||
)
|
||||
continue
|
||||
errors += 1
|
||||
exitcode = 1
|
||||
|
|
@ -871,6 +955,13 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
funcname,
|
||||
relpath,
|
||||
)
|
||||
annotate(
|
||||
"error",
|
||||
path.relative_to(CODE_DIR),
|
||||
funcdef.lineno,
|
||||
funcdef.body[0].lineno,
|
||||
"Missing docstring",
|
||||
)
|
||||
continue
|
||||
elif funcname in MISSING_DOCSTRINGS.get(relpath, ()):
|
||||
# This was previously a know function with a missing docstring.
|
||||
|
|
@ -907,6 +998,13 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
funcname,
|
||||
relpath,
|
||||
)
|
||||
annotate(
|
||||
"warning",
|
||||
path.relative_to(CODE_DIR),
|
||||
funcdef.lineno,
|
||||
funcdef.body[0].lineno,
|
||||
"Missing 'CLI Example:' in docstring",
|
||||
)
|
||||
continue
|
||||
errors += 1
|
||||
exitcode = 1
|
||||
|
|
@ -915,6 +1013,13 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
funcname,
|
||||
relpath,
|
||||
)
|
||||
annotate(
|
||||
"error",
|
||||
path.relative_to(CODE_DIR),
|
||||
funcdef.lineno,
|
||||
funcdef.body[0].lineno,
|
||||
"Missing 'CLI Example:' in docstring",
|
||||
)
|
||||
continue
|
||||
elif funcname in MISSING_EXAMPLES.get(relpath, ()):
|
||||
# This was previously a know function with a missing CLI example
|
||||
|
|
@ -948,6 +1053,19 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
funcdef.name,
|
||||
path.relative_to(CODE_DIR),
|
||||
)
|
||||
annotate(
|
||||
"warning",
|
||||
path.relative_to(CODE_DIR),
|
||||
funcdef.lineno,
|
||||
funcdef.body[0].lineno,
|
||||
"Wrong format in 'CLI Example:' in docstring.\n"
|
||||
"The proper format is:\n```"
|
||||
"CLI Example:\n"
|
||||
"\n"
|
||||
".. code-block:: bash\n"
|
||||
"\n"
|
||||
" salt '*' <insert example here>\n```",
|
||||
)
|
||||
continue
|
||||
finally:
|
||||
if contents != path.read_text():
|
||||
|
|
@ -957,6 +1075,11 @@ def check(ctx, files, check_proper_formatting=False, error_on_known_failures=Fal
|
|||
utils.warn("Found {} warnings", warnings)
|
||||
if exitcode:
|
||||
utils.error("Found {} errors", errors)
|
||||
if os.environ.get("GH_ACTIONS_ANNOTATE") and (warnings or errors):
|
||||
github_step_summary = os.environ.get("GITHUB_STEP_SUMMARY")
|
||||
if github_step_summary:
|
||||
with open(github_step_summary, "w", encoding="utf-8") as wfh:
|
||||
wfh.write(SUMMARY)
|
||||
utils.exit_invoke(exitcode)
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue