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

Add support for multiple licenses in the header config section #118

Merged
merged 2 commits into from Jun 29, 2022

Conversation

dave-tucker
Copy link
Contributor

@dave-tucker dave-tucker commented Jun 18, 2022

This allows for multiple license configurations to be contained
within the same .licenserc.yaml file. The intended use is for a project
that has multiple packages or modules with differing licenses.

To do this we introduce a ConfigV2 config format since it is not valid
YAML for a key to be either a dictionary or a sequence.

We try to parse as V2 first, and fall back to V1.

@wu-sheng wu-sheng requested a review from kezhenxu94 June 19, 2022 00:03
@wu-sheng wu-sheng added this to the 0.4.0 milestone Jun 19, 2022
@kezhenxu94
Copy link
Member

kezhenxu94 commented Jun 19, 2022

Hi @dave-tucker thanks for your efforts and good finding for new use case. Comment inline.

This allows for multiple license-eye configurations to be contained

within the same .licenserc.yaml file. The intended use is for a project

that has multiple packages or modules with differing licenses.

This is a perfect example to have multiple sections for header config while it's weird to allow multiple sections for the entire .licenserc.yaml content as stated below.

For header commands, the behaviour is to always evaluate every config

section in the config file. This was chosen since it seems convenient to

have a single command for checking all licenses in CI.

For deps commands, the --config-index flag is used to select a config,

with 0 (first) being default. This was chosen since deps can generate

a LICENSE file, which could not be valid should the license differ

between configurations.

Yes but --config-index doesn't help in that case where the project has different licenses per module. All dependencies' licenses should be included no matter which module introduces them so there is no need to have multiple dependency config sections. And the license of the project itself is complex so users should not use {{.LicenseContent}} in the template, instead they should compose the license content in LICENSE template themselves stating clearly which module is licensed under what license.

So my opinion is to allow both object (HeaderConfig) and array ([]HeaderConfig) for header config, object is to make it simple for users who doesn't have the case here and to be compatible for existing users (we have many users out there already and I don't want to break their CI), and array is to address the case you have. So the following two configs are both valid ( 1st is the same as existing format and 2nd is newly added )

header:
  content: ...
  paths: ...
  # ...
dependency: # the same as before 
header:
  - content: ...
    paths: pkg1
    # ...
  - content: ...
    paths: pkg2
    # ...
dependency: # the same as before

@dave-tucker dave-tucker changed the title Add support for multiple configuration sections Add support for multiple licenses in the header config section Jun 19, 2022
@dave-tucker
Copy link
Contributor Author

Comments addressed and PR updated. I'm not sure it's possible to express in YAML "this field could be a dictionary, or a sequence" so instead I opted to change Config -> ConfigV1, add a new ConfigV2 and then use an interface Config to ensure the two types are used can be used interchangeably. V2 is tried first, with a fallback to V1.

@wu-sheng
Copy link
Member

Hi, CI fails, please fix them first.

@dave-tucker dave-tucker force-pushed the multiple-config branch 2 times, most recently from bfdf58a to fcf7b78 Compare June 19, 2022 15:02
@dave-tucker
Copy link
Contributor Author

Sorry about that. I had an out-of-date golangci-lint version which made make lint throw lots of spurious errors.
Upgrading to v1.43.0 didn't work out either - hit panics with go-critic.
Eventually I had to use the golangci-lint docker image, so hopefully this should pass now 😓

@kezhenxu94
Copy link
Member

Sorry about that. I had an out-of-date golangci-lint version which made make lint throw lots of spurious errors.

Upgrading to v1.43.0 didn't work out either - hit panics with go-critic.

Eventually I had to use the golangci-lint docker image, so hopefully this should pass now 😓

Thanks! I will take another look soon.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
commands/deps_check.go Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

@dave-tucker Friendly ping.

This allows for multiple `license` configurations to be contained
within the same .licenserc.yaml file. The intended use is for a project
that has multiple packages or modules with differing licenses.

To do this we introduce a ConfigV2 config format since it is not valid
YAML for a key to be either a dictionary or a sequence.

We try to parse as V2 first, and fall back to V1.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker
Copy link
Contributor Author

Sorry for the delay. Review comments addressed.

Copy link
Member

@kezhenxu94 kezhenxu94 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 @dave-tucker , can you by the way share which repository will use this feature 😄 ?

@kezhenxu94 kezhenxu94 merged commit c343ca4 into apache:main Jun 29, 2022
@dave-tucker
Copy link
Contributor Author

Thank you @dave-tucker , can you by the way share which repository will use this feature smile ?

bpfman/bpfman#47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants