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.

Sign up for GitHub

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

Jump to bottom

Fix #3497: Add CI check to ensure maven_dependencies.textproto is up to date #3520

Merged
merged 71 commits into from
Aug 13, 2021
Merged

Fix #3497: Add CI check to ensure maven_dependencies.textproto is up to date #3520

merged 71 commits into from
Aug 13, 2021

Conversation

prayutsu
Copy link
Contributor

@prayutsu prayutsu commented Jul 24, 2021 •
edited

Explanation

Fixes #3497:
Added a CI check that ensures maven_dependencies.textproto is always up-to-date
All newly added test cases passing -
Screenshot from 2021-08-11 17-36-20

Screenshot from 2021-07-31 15-46-26

CI check failing because of incomplete list -
Screenshot from 2021-07-29 17-59-58

The new Maven Dependencies CI check failed in actual as Sparsh recently added some Kotlin dependency and thus maven_dependencies.textproto needed to be updated -
Screenshot from 2021-07-31 23-08-32

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.

@prayutsu
Call out dependency names when license details incomplete exception i…
dca89e1 
…s thrown
@prayutsu
Modify message to include rerunning the script after updating maven_d…
eacaab6 
…ependencies.textproto
@prayutsu
Correct typo
2dd9322
@prayutsu
Add suggested changes
68b8e85
@prayutsu
Add full stop
b2fdd31
@prayutsu
Correct comment sentence in versions.bzl
ebcc66b
@prayutsu
Add some more test cases and add suggested changes
7dcd7e4
@prayutsu
Merge branch 'develop' of github.com:oppia/oppia-android into callout…
0cd5ab3 
…-coordinate-name
@prayutsu
Merge branches 'callout-coordinate-name' and 'develop' of github.com:…
5d39b7f 
…oppia/oppia-android into callout-coordinate-name
@prayutsu
Separate all methods in a new file
8cbf5e1
@prayutsu
Correct imports
4df99c9
@prayutsu
Update to reflect changes of coordinate name
67e73e2
@prayutsu
Add CI check
ad04270
@prayutsu
Correct static_checks.yml
2f96498
@prayutsu
Remove empty line
408e55e
@prayutsu prayutsu changed the base branch from develop to callout-coordinate-name July 24, 2021 15:13
@prayutsu prayutsu changed the title Add CI check to ensure maven_dependencies.textproto is up to date Fix #3497: Add CI check to ensure maven_dependencies.textproto is up to date Jul 24, 2021
@prayutsu prayutsu marked this pull request as ready for review July 24, 2021 17:59
@prayutsu prayutsu requested a review from rt4914 July 24, 2021 18:00
@prayutsu
Copy link
Contributor Author

@anandwana001 @BenHenning @rt4914 PTAL for a first pass.

I'll test cases for the new files and KDocs for all the public members as soon as the structure of the new file is approved.

anandwana001
anandwana001 suggested changes Jul 25, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Thanks @prayutsu
Did the first pass. Review the nit things, as Bazel is not working for me so I am not able to run the script. Depending on @rt4914 to confirm the implementation. Once my bazel setup got fixed, I will review back again.

.github/workflows/static_checks.yml Outdated Show resolved Hide resolved
app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/ci/VerifyMavenDependenciesListIsUpToDate.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/ci/VerifyMavenDependenciesListIsUpToDate.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/ci/VerifyMavenDependenciesListIsUpToDate.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/common/MavenDependenciesListGenerator.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/common/MavenDependenciesListGenerator.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/common/MavenDependenciesListGenerator.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/common/MavenDependenciesListGenerator.kt Outdated Show resolved Hide resolved
scripts/src/java/org/oppia/android/scripts/common/MavenDependenciesListGenerator.kt Outdated Show resolved Hide resolved
@prayutsu prayutsu reopened this Aug 5, 2021
@BenHenning
Copy link
Sponsor Member

Hi. Will need to look at this tomorrow.

BenHenning
BenHenning reviewed Aug 11, 2021
View reviewed changes
Copy link
Sponsor 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.

Hi @prayutsu. I finished taking a full pass on the PR and had a few more comments. PTAL.

scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
.github/workflows/static_checks.yml Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Aug 11, 2021
@prayutsu prayutsu assigned BenHenning and unassigned prayutsu Aug 11, 2021
@prayutsu
Copy link
Contributor Author

@BenHenning PTAL.

BenHenning
BenHenning reviewed Aug 13, 2021
View reviewed changes
Copy link
Sponsor 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 @prayutsu. Generally LGTM, just had a few more comments. PTAL.

scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesListCheckTest.kt Outdated Show resolved Hide resolved
scripts/src/javatests/org/oppia/android/scripts/common/MavenDependenciesListGeneratorTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Aug 13, 2021
@prayutsu prayutsu assigned BenHenning and unassigned prayutsu Aug 13, 2021
@prayutsu prayutsu mentioned this pull request Aug 13, 2021
Merged
8 tasks
BenHenning
BenHenning approved these changes Aug 13, 2021
View reviewed changes
Copy link
Sponsor 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 @prayutsu. LGTM!

scripts/src/javatests/org/oppia/android/scripts/license/MavenDependenciesRetrieverTest.kt Outdated Show resolved Hide resolved
@BenHenning
Copy link
Sponsor Member

Merging since everyone approved & there are no outstanding comment threads.

@BenHenning BenHenning merged commit cfed5fe into develop Aug 13, 2021
GSoC 2021.6: Support for displaying copyright licenses automation moved this from In progress to Done Aug 13, 2021
@BenHenning BenHenning deleted the add-ci-check-to-ensure-maven_dependencies.textproto-is-up-to-date branch August 13, 2021 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

@anandwana001 anandwana001 anandwana001 approved these changes

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
No open projects
GSoC 2021.6: Support for displaying c...
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Github CI check to verify the dependency list is always up-to-date
5 participants
@prayutsu @anandwana001 @BenHenning @rt4914 @Sparsh1212