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 the Elastic license #15

Merged
merged 4 commits into from
Aug 3, 2018

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Aug 1, 2018

Description

Introduces support for multiple licenses through the --license flag, keeping the
current one (ASL2) as the default, and adding an Elastic option.

Closes #1

@exekias exekias force-pushed the add-elastic-license branch 2 times, most recently from 67b095e to e5e897a Compare August 1, 2018 11:16
@exekias
Copy link
Contributor Author

exekias commented Aug 1, 2018

something is wrong after rebasing, will have a look

@exekias exekias force-pushed the add-elastic-license branch 6 times, most recently from 8d08075 to b16d1e8 Compare August 1, 2018 16:06
Introduces support for multiple licenses, keeping the
current one (`ASL2`) as the default, and adding an `Elastic` option.

Signed-off-by: Carlos Pérez-Aradros Herce <[email protected]>
@exekias exekias force-pushed the add-elastic-license branch from b16d1e8 to 49b56ac Compare August 1, 2018 16:09
@exekias
Copy link
Contributor Author

exekias commented Aug 1, 2018

This should be ready now, sorry for the noise 😇

@ph ph requested a review from marclop August 1, 2018 19:25
Copy link
Collaborator

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Code looks good and functionality was much needed 🙇

Copy link

@ph ph left a comment

Choose a reason for hiding this comment

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

@exekias

I think we should either relax the strictness of the name or update the documentation to display the right license option. Using 'Elastic' with the right capitalization works but the doc mention 'elastic' or 'apache2' which should be 'ASL2'

 ❯ go-licenser -license elastic                                                                                                                                                                                                                                                                                                      
unkown license: elastic%                                                                                                                                                                                                                                                                                                                                                  
 

Copy link

@ph ph left a comment

Choose a reason for hiding this comment

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

small typo and a remark.

main.go Outdated
func run(args []string, license string, exclude []string, ext string, dry bool, out io.Writer) error {
header, ok := Headers[license]
if !ok {
return &Error{err: fmt.Errorf("unkown license: %s", license), code: errUnknownLicense}
Copy link

Choose a reason for hiding this comment

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

s/unkown/unknown/

Copy link

Choose a reason for hiding this comment

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

nit: Maybe we could add the valid licenses? This will make the error message easily actionable.

@exekias
Copy link
Contributor Author

exekias commented Aug 2, 2018

Thank you for the reviews @marclop @ph! I pushed a fix to the flag docs, also updated README

@exekias exekias force-pushed the add-elastic-license branch from 782e485 to 7768f16 Compare August 2, 2018 14:09
@exekias exekias force-pushed the add-elastic-license branch from 7768f16 to 0b3515e Compare August 2, 2018 14:22
Copy link

@ph ph left a comment

Choose a reason for hiding this comment

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

I've tested it with my local source code and it works, thanks @exekias!

@marclop should I just merge that or you want to do it?

@exekias exekias merged commit 0232478 into elastic:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants