[shac] Ignore changed files from different repos
If the Gerrit change being tested is in a different repository from the
repository being analyzed, don't consider any files affected. This
ensures we don't incorrectly add comments to files in the wrong
repository, which could happen if two repositories contained a file at
the same location.
I haven't seen any indication of this occurring yet, but it likely would
have come up eventually.
I also changed the `api.git.get_remotes()` function to return a list
instead of a generator to make it behave less surprisingly when used in
conjunction with `with api.context(cwd=...):`; when it returns a
generator, the underlying step doesn't run until the generator is
iterated over, which may be outside the intended context, as opposed to
when the function is called.
Change-Id: I0f5d622eb802589d4dd70863a1cd36e875b25005
Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/998512
Commit-Queue: Auto-Submit <auto-submit@fuchsia-infra.iam.gserviceaccount.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Reviewed-by: Ina Huh <ihuh@google.com>
diff --git a/recipe_modules/git/api.py b/recipe_modules/git/api.py
index bf4fae9..ef37255 100644
--- a/recipe_modules/git/api.py
+++ b/recipe_modules/git/api.py
@@ -1342,7 +1342,7 @@
"""Get the remotes configured for a repository."""
Remote = collections.namedtuple("Remote", "name url")
- for line in self.config(
+ lines = self.config(
"--get-regexp",
"^remote.*",
step_name="get remotes",
@@ -1353,7 +1353,10 @@
"remote.mirror.url https://fuchsia.googlesource.com/mirror\n"
"remote.mirror.url https://fuchsia.googlesource.com/mirror\n"
),
- ).stdout.splitlines():
+ ).stdout.splitlines()
+ remotes = []
+ for line in lines:
match = re.search(r"^remote.([^.]*).url\s+(.+)\s*$", line)
if match:
- yield Remote(name=match.group(1), url=match.group(2))
+ remotes.append(Remote(name=match.group(1), url=match.group(2)))
+ return remotes
diff --git a/recipe_modules/shac/api.py b/recipe_modules/shac/api.py
index 2453e87..ca0cfad 100644
--- a/recipe_modules/shac/api.py
+++ b/recipe_modules/shac/api.py
@@ -89,12 +89,6 @@
if step.retcode:
step.presentation.status = self.m.step.FAILURE
- bb_input = self.m.buildbucket.build.input
- changed_files = ()
- if self.m.buildbucket_util.is_tryjob and bb_input.gerrit_changes:
- with self.m.context(cwd=root):
- changed_files = set(self.m.git.get_changed_files("list affected files"))
-
comments = []
# Mapping from check name to list of error messages.
errors_by_check = collections.defaultdict(list)
@@ -155,6 +149,9 @@
]
comments.append(comment)
+ with self.m.step.nest("get changed files"):
+ changed_files = self._changed_files(root)
+
# Only include comments for changed files or special files like
# "/COMMIT_MESSAGE".
comments = [
@@ -190,7 +187,7 @@
)
if emit_comments and comments:
- gerrit_change = bb_input.gerrit_changes[0]
+ gerrit_change = self.m.buildbucket.build.input.gerrit_changes[0]
# List the comments that have already been added by previous
# shac runs to avoid emitting duplicate comments. This isn't
@@ -292,3 +289,24 @@
"end_line": end_line,
"end_character": max(region.end_column - 1, 0),
}
+
+ def _changed_files(self, root):
+ changes = self.m.buildbucket.build.input.gerrit_changes
+ if not self.m.buildbucket_util.is_tryjob or not changes:
+ return set()
+
+ with self.m.context(cwd=root):
+ remotes = self.m.git.get_remotes()
+ remotes = [r for r in remotes if r.name == "origin"]
+ assert remotes, "no remote with name 'origin'"
+
+ remote = remotes[0].url
+ gerrit_host = self.m.gerrit.host_from_remote_url(remote)
+ gerrit_project = self.m.gerrit.project_from_remote_url(remote)
+ if (changes[0].host, changes[0].project) != (gerrit_host, gerrit_project):
+ # Don't consider any files to have changed if the repo being checked
+ # is not the same repo that the pending change is in.
+ return set()
+
+ with self.m.context(cwd=root):
+ return set(self.m.git.get_changed_files())
diff --git a/recipe_modules/shac/tests/full.expected/change_in_different_repo.json b/recipe_modules/shac/tests/full.expected/change_in_different_repo.json
new file mode 100644
index 0000000..5d84f6d
--- /dev/null
+++ b/recipe_modules/shac/tests/full.expected/change_in_different_repo.json
@@ -0,0 +1,72 @@
+[
+ {
+ "cmd": [
+ "rdb",
+ "stream",
+ "--",
+ "[CLEANUP]/checkout_tmp_1/shac",
+ "check",
+ "--json-output",
+ "/path/to/tmp/json",
+ "--var",
+ "baz=quux",
+ "--var",
+ "foo=bar",
+ "--all",
+ "--entrypoint",
+ "other_shac.star"
+ ],
+ "cwd": "[CLEANUP]/checkout_tmp_1",
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "shac check",
+ "~followup_annotations": [
+ "@@@STEP_TEXT@Run `shac check --all --entrypoint other_shac.star` to reproduce locally.@@@",
+ "@@@STEP_LOG_LINE@proto.output@{}@@@",
+ "@@@STEP_LOG_END@proto.output@@@"
+ ]
+ },
+ {
+ "cmd": [],
+ "name": "get changed files"
+ },
+ {
+ "cmd": [
+ "git",
+ "config",
+ "--get-regexp",
+ "^remote.*"
+ ],
+ "cwd": "[CLEANUP]/checkout_tmp_1",
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "get changed files.get remotes",
+ "timeout": 300.0,
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
+ },
+ {
+ "name": "$result"
+ }
+]
\ No newline at end of file
diff --git a/recipe_modules/shac/tests/full.expected/fail.json b/recipe_modules/shac/tests/full.expected/fail.json
index 347d456..2f85d95 100644
--- a/recipe_modules/shac/tests/full.expected/fail.json
+++ b/recipe_modules/shac/tests/full.expected/fail.json
@@ -563,41 +563,6 @@
},
{
"cmd": [
- "git",
- "diff-tree",
- "--no-commit-id",
- "--name-only",
- "--find-renames=100%",
- "--diff-filter=rd",
- "-r",
- "-z",
- "--ignore-submodules=all",
- "HEAD"
- ],
- "cwd": "[CLEANUP]/checkout_tmp_1",
- "luci_context": {
- "realm": {
- "name": "fuchsia:try"
- },
- "resultdb": {
- "current_invocation": {
- "name": "invocations/build:8945511751514863184",
- "update_token": "token"
- },
- "hostname": "rdbhost"
- }
- },
- "name": "list affected files",
- "timeout": 300.0,
- "~followup_annotations": [
- "@@@STEP_LOG_LINE@files@[@@@",
- "@@@STEP_LOG_LINE@files@ \"src/script.py\"@@@",
- "@@@STEP_LOG_LINE@files@]@@@",
- "@@@STEP_LOG_END@files@@@"
- ]
- },
- {
- "cmd": [
"vpython3",
"-u",
"RECIPE_MODULE[recipe_engine::file]/resources/fileutil.py",
@@ -627,6 +592,72 @@
},
{
"cmd": [],
+ "name": "get changed files"
+ },
+ {
+ "cmd": [
+ "git",
+ "config",
+ "--get-regexp",
+ "^remote.*"
+ ],
+ "cwd": "[CLEANUP]/checkout_tmp_1",
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "get changed files.get remotes",
+ "timeout": 300.0,
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
+ },
+ {
+ "cmd": [
+ "git",
+ "diff-tree",
+ "--no-commit-id",
+ "--name-only",
+ "--find-renames=100%",
+ "--diff-filter=rd",
+ "-r",
+ "-z",
+ "--ignore-submodules=all",
+ "HEAD"
+ ],
+ "cwd": "[CLEANUP]/checkout_tmp_1",
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "get changed files.git diff-tree",
+ "timeout": 300.0,
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@",
+ "@@@STEP_LOG_LINE@files@[@@@",
+ "@@@STEP_LOG_LINE@files@ \"src/script.py\"@@@",
+ "@@@STEP_LOG_LINE@files@]@@@",
+ "@@@STEP_LOG_END@files@@@"
+ ]
+ },
+ {
+ "cmd": [],
"name": "ensure infra/tools/luci/gerrit/${platform}"
},
{
diff --git a/recipe_modules/shac/tests/full.expected/pass.json b/recipe_modules/shac/tests/full.expected/pass.json
index 9b59efd..25c36c7 100644
--- a/recipe_modules/shac/tests/full.expected/pass.json
+++ b/recipe_modules/shac/tests/full.expected/pass.json
@@ -37,6 +37,10 @@
]
},
{
+ "cmd": [],
+ "name": "get changed files"
+ },
+ {
"name": "$result"
}
]
\ No newline at end of file
diff --git a/recipe_modules/shac/tests/full.expected/too_many_comments.json b/recipe_modules/shac/tests/full.expected/too_many_comments.json
index 351fcf8..dc85acb 100644
--- a/recipe_modules/shac/tests/full.expected/too_many_comments.json
+++ b/recipe_modules/shac/tests/full.expected/too_many_comments.json
@@ -1128,6 +1128,36 @@
]
},
{
+ "cmd": [],
+ "name": "get changed files"
+ },
+ {
+ "cmd": [
+ "git",
+ "config",
+ "--get-regexp",
+ "^remote.*"
+ ],
+ "cwd": "[CLEANUP]/checkout_tmp_1",
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "get changed files.get remotes",
+ "timeout": 300.0,
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
+ },
+ {
"cmd": [
"git",
"diff-tree",
@@ -1153,9 +1183,10 @@
"hostname": "rdbhost"
}
},
- "name": "list affected files",
+ "name": "get changed files.git diff-tree",
"timeout": 300.0,
"~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@",
"@@@STEP_LOG_LINE@files@[@@@",
"@@@STEP_LOG_LINE@files@ \"src/script.py\"@@@",
"@@@STEP_LOG_LINE@files@]@@@",
diff --git a/recipe_modules/shac/tests/full.py b/recipe_modules/shac/tests/full.py
index cb4500d..bb0d7b6 100644
--- a/recipe_modules/shac/tests/full.py
+++ b/recipe_modules/shac/tests/full.py
@@ -31,10 +31,14 @@
def GenTests(api):
yield api.buildbucket_util.test("pass")
+ yield api.buildbucket_util.test(
+ "change_in_different_repo", repo="other_repo", tryjob=True
+ )
+
yield (
api.buildbucket_util.test("fail", tryjob=True, status="FAILURE")
+ api.git.get_changed_files(
- "list affected files",
+ "get changed files.git diff-tree",
["src/script.py"],
)
+ api.step_data(
@@ -252,7 +256,7 @@
yield (
api.buildbucket_util.test("too_many_comments", tryjob=True, status="FAILURE")
+ api.git.get_changed_files(
- "list affected files",
+ "get changed files.git diff-tree",
["src/script.py"],
)
+ api.step_data(
diff --git a/recipes/shac.expected/basic.json b/recipes/shac.expected/basic.json
index 446a9e1..e1eb9d2 100644
--- a/recipes/shac.expected/basic.json
+++ b/recipes/shac.expected/basic.json
@@ -823,6 +823,10 @@
]
},
{
+ "cmd": [],
+ "name": "get changed files"
+ },
+ {
"name": "$result"
}
]
\ No newline at end of file
diff --git a/recipes/shac.expected/shac_path.json b/recipes/shac.expected/shac_path.json
index 10d92d0..0484617 100644
--- a/recipes/shac.expected/shac_path.json
+++ b/recipes/shac.expected/shac_path.json
@@ -721,6 +721,10 @@
]
},
{
+ "cmd": [],
+ "name": "get changed files"
+ },
+ {
"name": "$result"
}
]
\ No newline at end of file
diff --git a/recipes/static_checks.expected/default_ci.json b/recipes/static_checks.expected/default_ci.json
index b1cae8d..b28073b 100644
--- a/recipes/static_checks.expected/default_ci.json
+++ b/recipes/static_checks.expected/default_ci.json
@@ -3752,6 +3752,13 @@
]
},
{
+ "cmd": [],
+ "name": "run shac checks.get changed files",
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
+ },
+ {
"name": "$result"
}
]
\ No newline at end of file
diff --git a/recipes/static_checks.expected/default_cq.json b/recipes/static_checks.expected/default_cq.json
index 1fbb386..b8c630b 100644
--- a/recipes/static_checks.expected/default_cq.json
+++ b/recipes/static_checks.expected/default_cq.json
@@ -4191,6 +4191,42 @@
]
},
{
+ "cmd": [],
+ "name": "run shac checks.get changed files",
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
+ },
+ {
+ "cmd": [
+ "git",
+ "config",
+ "--get-regexp",
+ "^remote.*"
+ ],
+ "cwd": "[START_DIR]/fuchsia",
+ "env": {
+ "FUCHSIA_DIR": "[START_DIR]/fuchsia"
+ },
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "run shac checks.get changed files.get remotes",
+ "timeout": 300.0,
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@2@@@"
+ ]
+ },
+ {
"cmd": [
"git",
"diff-tree",
@@ -4219,10 +4255,10 @@
"hostname": "rdbhost"
}
},
- "name": "run shac checks.list affected files",
+ "name": "run shac checks.get changed files.git diff-tree",
"timeout": 300.0,
"~followup_annotations": [
- "@@@STEP_NEST_LEVEL@1@@@",
+ "@@@STEP_NEST_LEVEL@2@@@",
"@@@STEP_LOG_LINE@files@[@@@",
"@@@STEP_LOG_LINE@files@ \"foo/bar/baz\",@@@",
"@@@STEP_LOG_LINE@files@ \"foo/baz\"@@@",
@@ -4273,6 +4309,42 @@
]
},
{
+ "cmd": [],
+ "name": "run shac checks in vendor/foo.get changed files",
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
+ },
+ {
+ "cmd": [
+ "git",
+ "config",
+ "--get-regexp",
+ "^remote.*"
+ ],
+ "cwd": "[START_DIR]/fuchsia/vendor/foo",
+ "env": {
+ "FUCHSIA_DIR": "[START_DIR]/fuchsia"
+ },
+ "luci_context": {
+ "realm": {
+ "name": "fuchsia:try"
+ },
+ "resultdb": {
+ "current_invocation": {
+ "name": "invocations/build:8945511751514863184",
+ "update_token": "token"
+ },
+ "hostname": "rdbhost"
+ }
+ },
+ "name": "run shac checks in vendor/foo.get changed files.get remotes",
+ "timeout": 300.0,
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@2@@@"
+ ]
+ },
+ {
"cmd": [
"git",
"diff-tree",
@@ -4301,10 +4373,10 @@
"hostname": "rdbhost"
}
},
- "name": "run shac checks in vendor/foo.list affected files",
+ "name": "run shac checks in vendor/foo.get changed files.git diff-tree",
"timeout": 300.0,
"~followup_annotations": [
- "@@@STEP_NEST_LEVEL@1@@@",
+ "@@@STEP_NEST_LEVEL@2@@@",
"@@@STEP_LOG_LINE@files@[@@@",
"@@@STEP_LOG_LINE@files@ \"foo/bar/baz\",@@@",
"@@@STEP_LOG_LINE@files@ \"foo/baz\"@@@",
