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

Added CLI option to read license file for inlets operator. #70

Closed
wants to merge 2 commits into from
Closed

Added CLI option to read license file for inlets operator. #70

wants to merge 2 commits into from

Conversation

anitsh
Copy link

@anitsh anitsh commented Mar 25, 2020 •
edited
Loading

Signed-off-by: Anit codeanit@gmail.com

CLI option license-file or lf added to read a inlets operator license file.

Description

CLI option license-file or lf added to read a inlets operator license file.

Motivation and Context

  • I have raised an issue to propose this change (64)
    The existing and previous option to read license key passed as a string is insecure. Hence, this featured was needed.

How Has This Been Tested?

  • Module has been developed as an standalone package
  • Unit test has been added to verify it works standalone.
  • Manual test are done by running the application is done.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • New application can be installed on ARM platform.
  • New application shows an error message if the architecture is not supported.

@anitsh
alexellis#61 Added CLI option to read license file for inlets operator.
7fc9923 
Signed-off-by: Anit <codeanit@gmail.com>
@anitsh
Removed commented codes
6dd0d21 
Signed-off-by: Anit <codeanit@gmail.com>
@anitsh
Copy link
Author

anitsh commented Mar 25, 2020

@Waterdrips Hope things are well. Could you please review?

@anitsh anitsh changed the title #61 Added CLI option to read license file for inlets operator. Added CLI option to read license file for inlets operator. Mar 25, 2020
@anitsh anitsh mentioned this pull request Mar 25, 2020
Closed
@alexellis
Copy link
Owner

Thanks for the PR. @Waterdrips did you see this ping?

alexellis
alexellis reviewed Mar 29, 2020
View reviewed changes
pkg/license/license.go
@@ -0,0 +1,53 @@
// Package license reads a license file and
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need this package. Can you delete it please?

ioutil.Readfile is fine.

Copy link
Author

Choose a reason for hiding this comment

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

Noted!
@alexellis, I will make the changes as recommended. Thanks.

alexellis
alexellis reviewed Mar 29, 2020
View reviewed changes
pkg/license/license_test.go
@@ -0,0 +1,49 @@
package license
Copy link
Owner

Choose a reason for hiding this comment

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

Not required, please delete.

Copy link
Author

Choose a reason for hiding this comment

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

Noted!
Thanks @alexellis.

alexellis
alexellis reviewed Mar 29, 2020
View reviewed changes
cmd/apps/inletsoperator_app.go
@@ -161,6 +162,15 @@ func MakeInstallInletsOperator() *cobra.Command {
overrides["inletsProLicense"] = val
}

if licenseFile, _ := command.Flags().GetString("license-file"); len(licenseFile) > 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

Please just use https://golang.org/pkg/io/ioutil/#ReadFile

Copy link
Author

Choose a reason for hiding this comment

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

@alexellis Thanks. Will do.

alexellis
alexellis reviewed Mar 29, 2020
View reviewed changes
cmd/apps/inletsoperator_app.go
inletsOperator.Flags().String("pro-client-image", "", "Docker image for inlets-pro's client")
inletsOperator.Flags().Bool("helm3", true, "Use helm3, if set to false uses helm2")
inletsOperator.Flags().StringArray("set", []string{}, "Use custom flags or override existing flags \n(example --set=image=org/repo:tag)")
inletsOperator.Flags().String("license-file", "lf", "The text file containing license key for inlets-pro")
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure you can use a two letter alias of lf, was this tested?

Copy link
Author

Choose a reason for hiding this comment

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

@alexellis, It was tested passing value to the parameter name. I will double check while making changes. Thanks.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's set it to something else, a single character. is -f available? or -l?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Thanks.

alexellis
alexellis requested changes Mar 29, 2020
View reviewed changes
Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Bit too complicated. Please can you see the notes and make sure you've tested.

alexellis
alexellis requested changes Apr 2, 2020
View reviewed changes
Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Please see comments.

@alexellis alexellis mentioned this pull request Apr 3, 2020
Merged
12 tasks
@alexellis
Copy link
Owner

Thanks for having a go at this PR, we needed it a little sooner, but you were not to know.

Fixed via: #83

Please feel free to stop by again and we'll see if we can find something else for you to get a quick win with.

@alexellis alexellis closed this Apr 3, 2020
@anitsh
Copy link
Author

anitsh commented Apr 3, 2020

Will do @alexellis. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexellis alexellis alexellis requested changes

@Waterdrips Waterdrips Awaiting requested review from Waterdrips

Assignees
No one assigned
Labels
new-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anitsh @alexellis