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

Rework license scripts #185

Merged
merged 11 commits into from Apr 8, 2019
Merged

Rework license scripts #185

merged 11 commits into from Apr 8, 2019

Conversation

ordovicia
Copy link
Collaborator

@ordovicia ordovicia commented Apr 4, 2019 •
edited

This is an attempt to reworking #180.

I rewrote the license scripts in Python to

  • reduce the redundant codes,
  • validate the full string of the license header, and
  • make the script reserve shebangs and file permissions.

This PR makes a minor modification on *_k8s.go files.
The current files have "(C)" strings as well as "copyright", so I removed the redundant strings.

@ordovicia ordovicia requested a review from dtaniwaki April 4, 2019 08:15
@ordovicia ordovicia mentioned this pull request Apr 4, 2019
Closed
dtaniwaki
dtaniwaki reviewed Apr 4, 2019
View reviewed changes
Copy link
Contributor

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

Writing the license operations in Python a good idea! I made a few comments about minor things. Would you check them?

Makefile
@@ -29,7 +29,3 @@ e2e:
coverage:
go test -covermode=count -coverprofile=profile.cov $(shell go list ./... | grep -v /vendor/)
go tool cover -func=profile.cov

.PHONY: check-license
check-license:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This target is not used.
And to validate license headers, we can simply do $ python3 ./scripts/license/check.py.
I think there is no need to use Make.

We can re-add this target later if we need.

scripts/check_license.py Outdated Show resolved Hide resolved
dtaniwaki
dtaniwaki previously approved these changes Apr 4, 2019
View reviewed changes
Copy link
Contributor

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

LGTM!

dtaniwaki
dtaniwaki approved these changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@dtaniwaki dtaniwaki left a comment

Choose a reason for hiding this comment

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

Thank you!

@ordovicia ordovicia merged commit f8788f5 into pfnet-research:master Apr 8, 2019
@ordovicia ordovicia deleted the rework-license-scripts branch April 8, 2019 03:18
@ordovicia ordovicia added enhancement Category: Enhancement on existing features meta Area: Meta issues and PRs on this project infra Area: Issues and PRs related to the development infrastructure (e.g. CI, license scripts) and removed meta Area: Meta issues and PRs on this project labels Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtaniwaki dtaniwaki dtaniwaki approved these changes

Assignees
No one assigned
Labels
enhancement Category: Enhancement on existing features infra Area: Issues and PRs related to the development infrastructure (e.g. CI, license scripts)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ordovicia @dtaniwaki