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

*: Integrate SkyWalking-eyes to check license headers in CI and fix licenses headers #27198

Merged
merged 19 commits into from Aug 16, 2021

Conversation

fgksgf
Copy link
Contributor

@fgksgf fgksgf commented Aug 13, 2021

What problem does this PR solve?

Issue Number: close #26956

Problem Summary:

  • currently, we check the license header manually. We can use CI to automate it.
  • Many licnese headers have one less sentence compared to the standard one.

What is changed and how it works?

What's Changed:

  • add SkyWalking-eyes github action
  • license headers in files that are commented with // and #

How it Works: it will check the license header in CI

Check List

  • No code, new CI phase

Release note

None

      .vol/
      Applications/
      Library/
      System/
      Users/
      Volumes/
      bin/
      cores/
      dev/
      opt/
      private/
      sbin/
      usr/
      .VolumeIcon.icns
      .file
      etc
      home
      tmp
      var
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 13, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • bb7133
  • zz-jason

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2021

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 13, 2021
@github-actions github-actions bot added the sig/planner SIG: Planner label Aug 13, 2021
@tisonkun tisonkun self-requested a review August 13, 2021 08:02
@tisonkun
Copy link
Contributor

@fgksgf thanks for submitting this PR! Nice work.

CI failed seems due to too many debug logs, please take a look https://github.com/pingcap/tidb/pull/27198/checks?check_run_id=3319959872

@jyz0309
Copy link
Contributor

jyz0309 commented Aug 14, 2021

ERROR the following files don't have a valid license header: 
executor/show_placement.go
executor/show_placement_test.go
meta/main_test.go
testkit/testdata/testdata.go
util/chunk/main_test.go
util/math/main_test.go
util/printer/main_test.go
util/ranger/main_test.go 

@fgksgf
Copy link
Contributor Author

fgksgf commented Aug 15, 2021

ERROR the following files don't have a valid license header: 
executor/show_placement.go
executor/show_placement_test.go
meta/main_test.go
testkit/testdata/testdata.go
util/chunk/main_test.go
util/math/main_test.go
util/printer/main_test.go
util/ranger/main_test.go 

Because new changes with invalid license headers are continuously merged into the master...

@tisonkun
Copy link
Contributor

@fgksgf let's try to schedule a fast review to avoid too much conflict and follow up. Since CI pass for now, we can start reviewing.

@tisonkun
Copy link
Contributor

@fgksgf you may try to rearrange commits to factor out all find-and-replace in one commit so that we can do a real review.

@tisonkun
Copy link
Contributor

/run-check_dev_2

@feitian124
Copy link
Contributor

nice work👍

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM.

For further reviewers, the related non-trivial commits are

the related files are

  • .github/workflows/license-checker
  • .licenserc.yaml

@ti-chi-bot
Copy link
Member

@tisonkun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM.

For further reviewers, the related non-trivial commits are

the related files are

  • .github/workflows/license-checker
  • .licenserc.yaml

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@tisonkun
Copy link
Contributor

tisonkun commented Aug 16, 2021

@zz-jason @fgksgf I think it is not a blocker for this PR. We can get this PR merged and avoid further conflict by new files with wrong header. Create a follow up issue to evaluate move the config file out of root.

@zz-jason
Copy link
Member

could you put .licenserc.yaml to the .github/workflows folder to keep the tidb folder simple?

Good idea, I will file a new PR to make the upstream support this feature.

How long it takes to support this feature in the upstream github action? If it doesn't hard, I think we can wait for it.

@tisonkun
Copy link
Contributor

@zz-jason make sense. I just notice that the upstream PR get merged apache/skywalking-eyes#57

@wu-sheng
Copy link

I just merged that PR to upstream. It should be able to use here now.

@fgksgf
Copy link
Contributor Author

fgksgf commented Aug 16, 2021

Ok, i will update it.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 16, 2021
@tisonkun
Copy link
Contributor

/assign @zz-jason

Could you please help on merging this pr?

@tisonkun tisonkun added the sig/sql-infra SIG: SQL Infra label Aug 16, 2021
@tisonkun
Copy link
Contributor

/run-check_dev_2

@bb7133
Copy link
Member

bb7133 commented Aug 16, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: c43a768

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 16, 2021
@ti-chi-bot ti-chi-bot merged commit 7755d25 into pingcap:master Aug 16, 2021
@fgksgf fgksgf deleted the license branch August 16, 2021 10:52
@tisonkun
Copy link
Contributor

cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate SkyWalking-eyes to check license headers in CI
9 participants