Skip to content

Commit

Permalink
Fix #5312: Make todo open check locally runnable [Blocked: #5313] (#5315
Browse files Browse the repository at this point in the history
)

## Explanation
Fixes #5312

This updates the TODO open check script to be locally runnable rather
than requiring the developer to manually download the list of issues
from GitHub to analyze. This simplifies things and allows the script to
be easily run within the ``static_checks.sh`` script. This is being done
via interacting directly with GitHub using its RESTful API (see
https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-repository-issues)
in conjunction with the user's local auth token used to set up their
``gh`` tool (which needs to be set up, hence the changes to the wiki
documentation and clear error messages from the new ``GitHubClient``
utility).

To further simplify things, a ``regenerate`` mode was added to
regenerate the TODO exemptions textproto file (which is helpful for
#4929 hence why this comes before that PR).

The new command syntax to perform the TODO check is:

```sh
bazel run //scripts:todo_open_check -- <path_to_dir_root> <path_to_proto_binary> [regenerate]
```

With a specific example to just perform the check:

```sh
bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb
```

And an example to also perform regeneration:

```sh
bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb regenerate
```

Some other things specifically to note:
- TODO exemptions needed to be updated in this PR due to TODO utility &
test changes. The new file was created using the new regenerate
functionality.
- The TODO check has been added to the end of ``static_checks.sh``.
- The GitHub CI workflow was updated to use the new script syntax
appropriately.
- This is the first time scripts have been updated to integrate with
Retrofit, and this setup is going to be reused in the future for other
services.
- The data model for issues has been updated to better represent the
remote data structure.
- Moshi is being used along with Retrofit for an easier interaction with
GitHub as a remote endpoint. All of this has been wrapped in a new
``GitHubClient``.
- ``GitHubClient`` is designed to download all issues regardless of
length (whereas before the manual download step was limited to the first
2000 issues of the repository) using pagination.
- New tests were added to verify the regenerate flow (and properly set
up the mock OkHttp server since the script now relies on an HTTP
endpoint to download the GitHub issues itself).
- ``GitHubIssue`` is exempted from tests since it's just a basic data
structure, so there's no specific logic to test.
- ``GitHubService`` is exempted from tests since it's a template to
generate code via Retrofit's annotation processor, so there's no
specific logic to test.
- All scripts proto libraries were updated to use normal Java (rather
than Java lite) generation to provide text format support.
- The file paths included in ``TodoOpenCheck``'s output has been
simplified to be relative to the repository root rather than using
absolute paths (for parity with many other app scripts).
- Since ``GitHubClient``'s tests required interacting with ``gh``, a new
``FakeCommandExecutor`` was added (along with its own tests) to provide
support for orchestrating local utilities. This may be useful in other
tests in the future, though some of those script tests intentionally
integrate with environment commands like ``git`` and ``bazel``.

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
This is an infrastructure-only PR.

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Co-authored-by: Sean Lip <[email protected]>
  • Loading branch information
3 people authored Mar 20, 2024
1 parent 14a43d1 commit 5963eaa
Show file tree
Hide file tree
Showing 36 changed files with 2,099 additions and 685 deletions.
3 changes: 1 addition & 2 deletions .github/workflows/static_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,7 @@ jobs:
env:
GITHUB_TOKEN: ${{ github.token }}
run: |
gh issue list --limit 2000 --repo oppia/oppia-android --json number > $(pwd)/open_issues.json
bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb open_issues.json
bazel run //scripts:todo_open_check -- $(pwd) scripts/assets/todo_open_exemptions.pb
- name: String Resource Validation Check
if: always()
Expand Down
2 changes: 2 additions & 0 deletions scripts/assets/test_file_exemptions.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,8 @@ exempted_file_path: "instrumentation/src/java/org/oppia/android/instrumentation/
exempted_file_path: "instrumentation/src/java/org/oppia/android/instrumentation/testing/EndToEndTestHelper.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/CommandExecutor.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/CommandResult.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/model/GitHubIssue.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/common/remote/GitHubService.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/license/LicenseFetcher.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/license/LicenseFetcherImpl.kt"
exempted_file_path: "scripts/src/java/org/oppia/android/scripts/license/model/CopyrightLicense.kt"
Expand Down
Loading

0 comments on commit 5963eaa

Please sign in to comment.