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

Fix #1736: Add Bazel Linter #2048

Merged
merged 30 commits into from
Feb 13, 2021

Conversation

anandwana001
Copy link
Contributor

@anandwana001 anandwana001 commented Oct 27, 2020

Explanation

Fixes #1736: added linter for bazel

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@anandwana001 anandwana001 marked this pull request as draft October 27, 2020 05:12
app/BUILD.bazel Outdated Show resolved Hide resolved
@anandwana001 anandwana001 self-assigned this Oct 31, 2020
@anandwana001 anandwana001 marked this pull request as ready for review November 13, 2020 11:12
@anandwana001 anandwana001 removed their assignment Nov 13, 2020
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

LGTM!

@BenHenning
Copy link
Member

Will need to take a look at this tomorrow. Sorry for the delay.

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! Took a pass.

WORKSPACE Outdated Show resolved Hide resolved
app/BUILD.bazel Show resolved Hide resolved
app/test_with_resources.bzl Outdated Show resolved Hide resolved
app/test_with_resources.bzl Show resolved Hide resolved
model/src/main/proto/format_import_proto_library.bzl Outdated Show resolved Hide resolved
oppia_android_test.bzl Outdated Show resolved Hide resolved
oppia_android_test.bzl Outdated Show resolved Hide resolved
@anandwana001 anandwana001 marked this pull request as draft December 18, 2020 12:23
@BenHenning
Copy link
Member

That sounds fine @anandwana001. Thanks for the initial investigation work. Since we're taking that approach, please make sure that:

  1. There's an issue tracking specific pre-push hook support for Buildifier
  2. That this PR is updated to not include pre-push hook stuff
  3. We have sufficiently captured the things that worked & didn't work from our investigation, and the initial solution that was tried so that we can continue iterating on that after this PR is merged

@Luffy18346
Copy link
Contributor

@anandwana001 PTAL

@anandwana001
Copy link
Contributor Author

I will be updating this pr by this week

@Luffy18346
Copy link
Contributor

I will be updating this pr by this week

Ok, thanks

@anandwana001
Copy link
Contributor Author

anandwana001 commented Feb 4, 2021

pre-push tracking issue - #2107
Updated the description as well, will continue the work from thereafter merging this PR.

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001. Just had one comment about how we handle the CI linting check. PTAL.

@@ -53,7 +53,7 @@ jobs:
run: ./buf check lint --input=model/src/main/proto --input-config buf.yaml && echo "Protobuf lint check completed successfully"

- name: Bazel lint check
run: ./buildifier --lint=warn --mode=check --warnings=all -r app data domain model testing utility tools BUILD.bazel WORKSPACE oppia_android_test.bzl
run: ./buildifier --lint=warn --mode=check --warnings=-native-android,+out-of-order-load,+unsorted-dict-items -r app data domain model testing utility tools BUILD.bazel WORKSPACE oppia_android_test.bzl && echo "Bazel lint check completed successfully"
Copy link
Member

Choose a reason for hiding this comment

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

To clarify: I'm actually suggesting we move to the pattern of running our linters via scripts to avoid duplicating the code between the workflow file & the script. I think adding the pre-push hook in a subsequent PR is fine, but can we also move this to a linter script in this PR to establish that new pattern?

@BenHenning BenHenning assigned anandwana001 and unassigned BenHenning Feb 5, 2021
@anandwana001 anandwana001 changed the title Fix #1736: Add Bazel Linter Fix #1736: Add Bazel Linter [Blocked #2101] Feb 5, 2021
@anandwana001 anandwana001 changed the title Fix #1736: Add Bazel Linter [Blocked #2101] Fix #1736: Add Bazel Linter Feb 11, 2021
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @anandwana001! This LGTM.

@BenHenning
Copy link
Member

BenHenning commented Feb 12, 2021

Looks like a rare test infra flake. Restarting.

@anandwana001 anandwana001 enabled auto-merge (squash) February 13, 2021 03:52
@anandwana001 anandwana001 merged commit 35f50c5 into oppia:develop Feb 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bazel Linter to the project
5 participants