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 #3498: Add CI check to ensure license text is never checked into Git #3632

Merged
merged 91 commits into from
Aug 14, 2021
Merged

Fix #3498: Add CI check to ensure license text is never checked into Git #3632

merged 91 commits into from
Aug 14, 2021

Conversation

prayutsu
Copy link
Contributor

@prayutsu prayutsu commented Aug 1, 2021 •
edited

Explanation

Fixes #3498
Added a new script that will run on CI and verify that the version of third_party_dependencies.xml generated by GenerateLicenseTexts.kt is never checked into Git.

Screenshot -
Screenshot from 2021-08-01 20-57-14

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
Correct visibility of bazel targets and delete one argument from the …
9d6ef91 
…script
@prayutsu
Correct path in exemptions file
2ff95b8
@prayutsu
Add import for LicenseFetcher in test file
c64140b
@prayutsu
Add suggested changes
8a5dc1f
@prayutsu
Add suggested change
36b5921
@prayutsu
Merge branch 'develop' of github.com:oppia/oppia-android into add-ci-…
9ea924d 
…check-to-ensure-maven_dependencies.textproto-is-up-to-date
@prayutsu
correct kdoc
1718b76
@prayutsu
Use associateWith instead of associateTo
3f32180
@prayutsu
Merge branch 'develop' of github.com:oppia/oppia-android into add-ci-…
26eb0fc 
…check-to-ensure-maven_dependencies.textproto-is-up-to-date
@prayutsu
Resolve merge conflicts
17d2020
@prayutsu
Resolve conflicts
2f47260
@prayutsu
Merge branch 'develop' of github.com:oppia/oppia-android into callout…
9e2c8fe 
…-coordinate-name
@prayutsu
Merge branch 'callout-coordinate-name' of github.com:oppia/oppia-andr…
f034916 
…oid into add-ci-check-to-ensure-maven_dependencies.textproto-is-up-to-date
@prayutsu
Use associateWith instead of associateTo
d328a8c
@prayutsu
Add suggested changes
ab95715
@prayutsu prayutsu changed the title Fix #3498: Add CI check to ensure license text is never checked into Git [BLOCKED ON #3520] Fix #3498: Add CI check to ensure license text is never checked into Git Aug 13, 2021
BenHenning
BenHenning requested 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. Marking as request changes per your comment. Will double check & re-approve once #3520 is merged.

Base automatically changed from add-ci-check-to-ensure-maven_dependencies.textproto-is-up-to-date to develop August 13, 2021 09:15
@BenHenning
Copy link
Sponsor Member

@prayutsu please bring this up-to-date with develop now that its based PR is merged & assign back.

@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Aug 13, 2021
@oppiabot oppiabot bot removed the PR: LGTM label Aug 13, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 13, 2021

Hi, @prayutsu, the LGTM Label has been removed because the changes were requested on this PR. Thanks!.

@prayutsu
Copy link
Contributor Author

@BenHenning PTAL.

@prayutsu prayutsu assigned BenHenning and unassigned prayutsu Aug 13, 2021
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! Just noticed one thing that I missed earlier, but otherwise this LGTM.

scripts/src/javatests/org/oppia/android/scripts/license/LicenseTextsCheckTest.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
Copy link
Contributor Author

@BenHenning PTAL.

BenHenning
BenHenning approved these changes Aug 13, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment •
edited

Choose a reason for hiding this comment

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

Thanks @prayutsu. LGTM!

Can't enable auto-merge since I need to verify that the test change is passing (Bazel tests do not currently block PR submission).

@BenHenning BenHenning enabled auto-merge (squash) August 13, 2021 17:14
@BenHenning
Copy link
Sponsor Member

@prayutsu please assign back when CI is passing or merge yourself (but only if all CI checks are passing since some, such as the Bazel tests, are currently not required per our GitHub config).

@BenHenning BenHenning assigned prayutsu and unassigned BenHenning Aug 13, 2021
@oppiabot oppiabot bot added the PR: LGTM label Aug 13, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 13, 2021

Hi @prayutsu, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@BenHenning BenHenning merged commit 7aa1750 into develop Aug 14, 2021
GSoC 2021.6: Support for displaying copyright licenses automation moved this from In progress to Done Aug 14, 2021
@BenHenning BenHenning deleted the ci-check-to-verify-license-texts-are-never-checked-into-git branch August 14, 2021 03:06
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

Assignees

@prayutsu prayutsu

Labels
PR: LGTM
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.

Add CI check to ensure that the actual license texts can’t be accidentally checked into Git
3 participants
@prayutsu @BenHenning @rt4914