-
Notifications
You must be signed in to change notification settings - Fork 920
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
[CodeHealth] version_up
command-line tool
#27817
base: master
Are you sure you want to change the base?
Conversation
dce6c5b
to
7534f41
Compare
176bfc8
to
a7bfafc
Compare
b770f9c
to
f19adad
Compare
Setting |
6db60e8
to
2c8dc92
Compare
import json | ||
from pathlib import Path | ||
import re | ||
import subprocess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] Consider possible security implications associated with subprocess module.
Source: https://semgrep.dev/r/gitlab.bandit.B404
Cc @thypon @kdenhartog
_run_git('add', '-u', '*.patch') | ||
""" | ||
cmd = ['git'] + list(cmd) | ||
return subprocess.check_output(cmd).strip().decode('utf-8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reported by reviewdog 🐶
[semgrep] Python possesses many mechanisms to invoke an external executable. However,
doing so may present a security issue if appropriate care is not taken to
sanitize any user provided or variable input. This plugin test is part of a
family of tests built to check for process spawning and warn appropriately.
Specifically, this test looks for the spawning of a subprocess without the
use of a command shell. This type of subprocess invocation is not
vulnerable to shell injection attacks, but care should still be taken to
ensure validity of input.
Source: https://semgrep.dev/r/gitlab.bandit.B603
Cc @thypon @kdenhartog
script/version_up.py
Outdated
text=True, | ||
check=False) | ||
|
||
# This is a regex to match the json output of the patches that failed to apply. No test was conduct regarding inner arrays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really clear about this comment - it says that the pattern is to match failures, but then if it doesn't match anything we print 'No patches to apply'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex is merely used to check if JSON array list was produced because --print-patch-failures-in-json
doesn't always produce a JSON summary, if nothing failed to apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure on the script name itself. Maybe we can use something more descriptive like chromium_version_update.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also recommend moving this script into //brave/tools/cr_rebase
or some other dir in //brave/tools/
.
we should stop adding new stuff into //brave/script
, it's already a pile of everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this thread open for now to see if any more suggestions are provided.
2c8dc92
to
71a7ca1
Compare
This PR introduces a command line tool to assist with version upgrade tasks. This initial version supports the creation of the four basic changes necessary to get a new brave version building. This script is capable of a) updating the version number for Chromium, b) perform basic conflict resolution, c) create the "Update Patches" change for the new version, d) create the "Updated strings" change. This tool is being introduced as a starting point for making the daily "canary" tasks easier, as well as providing to provide more robust infra support for these tasks and for version bumps. This tool is should be usuable both at CI and locally. A basic run: ``` script/version_up.py --previous=origin/master --to=135.0.7035.1 ``` Changes to `.gitignore` are being introduced as this local file is required when the tool has give up on conflict resolution and wait for the user to resolve them. Resolves brave/brave-browser#44244
71a7ca1
to
f22770a
Compare
This PR introduces a command line tool to assist with version upgrade tasks. This initial version supports the creation of the four basic changes necessary to get a new brave version building.
This script is capable of a) updating the version number for Chromium, b) perform basic conflict resolution, c) create the "Update Patches" change for the new version, d) create the "Updated strings" change.
This tool is being introduced as a starting point for making the daily "canary" tasks easier, as well as providing to provide more robust infra support for these tasks and for version bumps.
This tool is should be usuable both at CI and locally.
A basic run:
Changes to
.gitignore
are being introduced as this local file is required when the tool has give up on conflict resolution and wait for the user to resolve them.Resolves brave/brave-browser#44244
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: