Skip to content

Refactor collecting-PR script for release note #1951

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

Merged
merged 4 commits into from
Nov 2, 2021
Merged

Conversation

nateanl
Copy link
Member

@nateanl nateanl commented Nov 1, 2021

@mthrok
Copy link
Collaborator

mthrok commented Nov 1, 2021

Looking at the code it is offending the bandit, it looks like the subprocess.Popen(command, ...) is causing the issue.

Before disabling the bandit, can you try changing the following commands into list of string?
I think then, we can remove shell=True.

cmd = f"git log -n 1 --pretty=format:%s {commit_hash}"

cmd = f"git merge-base {base_version} {new_version}"

cmd = f"git log --reverse --oneline {merge_base}..{new_version}"

@mthrok
Copy link
Collaborator

mthrok commented Nov 1, 2021

The git command should work just fine without shell=True, like the following

audio/setup.py

Lines 84 to 86 in 184466a

sha = _run_cmd(['git', 'rev-parse', 'HEAD'])
branch = _run_cmd(['git', 'rev-parse', '--abbrev-ref', 'HEAD'])
tag = _run_cmd(['git', 'describe', '--tags', '--exact-match', '@'])

@nateanl nateanl changed the title Ignore bandit for release notes scripts Fix bandit test failure for release notes scripts Nov 2, 2021
@carolineechen
Copy link
Contributor

thanks for looking into and working on this!

cmd = f"git log -n 1 --pretty=format:%s {commit_hash}"
ret, out, err = run(cmd)
return out if ret == 0 else None
cmd = ['git', 'log', '-n', '1', f'--pretty=format:%s {commit_hash}']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is different from what it used to be.

Suggested change
cmd = ['git', 'log', '-n', '1', f'--pretty=format:%s {commit_hash}']
cmd = ['git', 'log', '-n', '1', '--pretty=format:%s', f'{commit_hash}']

@mthrok
Copy link
Collaborator

mthrok commented Nov 2, 2021

@nateanl Can you make a small PR for the part only related to shell=True so that we can merge that one first quickly? That will suppress the bandit failure on other PRs.

@nateanl nateanl changed the title Fix bandit test failure for release notes scripts Refactor collecting-PR script for release note Nov 2, 2021
@@ -1,13 +1,18 @@
"""Collect the PRs between the current and previous releases and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Collect the PRs between the current and previous releases and
"""Collect the PRs between two specified tags or commits and

@@ -109,7 +109,10 @@ def get_commits_between(base_version, new_version):


def _parse_args(args):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _parse_args(args):
def _parse_args(args=None):

return subprocess.check_output(cmd).strip()
except Exception:
return None
return subprocess.check_output(cmd).decode('utf-8').strip()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This would fail if the commit message is not encoded with utf-8. (i.e. if user configures their git with non-uff commit message https://www.git-tower.com/help/guides/faq-and-tips/faq/encoding/windows)

The script needs to handle such case, while logging the failure and keep processing the rest of commits.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We need a way to detect the correct encoding from the result of check_output.
  2. The for loop in the main function has to handle error properly and notify the user.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will follow-up the case in the future PR.

@nateanl nateanl merged commit 42e6797 into pytorch:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants