Skip to content
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

tools: ignore ./bazel-* in check_repositories.sh #7239

Closed
wants to merge 1 commit into from

Conversation

rebello95
Copy link
Contributor

We currently ignore ./bazel-* in the check_format.py script, but not here. Adding it here as an ignored directory as well to prevent lint failures when using envoy as a submodule, such as the following:

  Checking repositories definitions
./bazel-envoy-mobile/external/bazel_tools/tools/build_defs/repo/git.bzl:68:        remote = ctx.attr.remote,
./bazel-envoy-mobile/external/bazel_tools/tools/build_defs/repo/git.bzl:96:    actual_commit = ctx.execute([
Using git repositories is not allowed.
To ensure that all dependencies can be stored offline in distdir, only HTTP repositories are allowed.

Signed-off-by: Michael Rebello mrebello@lyft.com

Risk Level: Low
Testing: Done locally

We currently ignore `./bazel-*` in the `check_format.py` script, but not here. Adding it here as an ignored directory as well to prevent lint failures when using `envoy` as a submodule, such as the following:

```
  Checking repositories definitions
./bazel-envoy-mobile/external/bazel_tools/tools/build_defs/repo/git.bzl:68:        remote = ctx.attr.remote,
./bazel-envoy-mobile/external/bazel_tools/tools/build_defs/repo/git.bzl:96:    actual_commit = ctx.execute([
Using git repositories is not allowed.
To ensure that all dependencies can be stored offline in distdir, only HTTP repositories are allowed.
```

Signed-off-by: Michael Rebello <mrebello@lyft.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. Quick question.

/wait

@@ -4,7 +4,7 @@ set -eu

# Check whether any git repositories are defined.
# Git repository definition contains `commit` and `remote` fields.
if grep -nr "commit =\|remote =" --include=*.bzl .; then
if grep -nr "commit =\|remote =" --include=*.bzl --exclude=./bazel-* .; then
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for working on this. From my quick look, I don't think we want to exclude this because this seems to pass OK upstream so we must be pulling in something into our repo which is causing this to fail. I think either we should figure out how to fix our repo or potentially make this exclusion list available via an ENV_VARIABLE that we can set somehow? WDYT?

@rebello95
Copy link
Contributor Author

Closing in favor of #7245

@rebello95 rebello95 closed this Jun 12, 2019
@rebello95 rebello95 deleted the repositories-script branch June 12, 2019 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants