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

fix: Bind kube client configuration flags before instantiating a kube client #42

Merged
merged 1 commit into from
Aug 21, 2019
Merged

fix: Bind kube client configuration flags before instantiating a kube client #42

merged 1 commit into from
Aug 21, 2019

Conversation

danielpacak
Copy link
Contributor

Resolves: #41

@codecov-io
Copy link

codecov-io commented Aug 16, 2019 •
edited
Loading

Codecov Report

Merging #42 into master will decrease coverage by 0.58%.
The diff coverage is 6.89%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #42      +/-   ##
=========================================
- Coverage   75.38%   74.8%   -0.59%     
=========================================
  Files           5       5              
  Lines         390     381       -9     
=========================================
- Hits          294     285       -9     
  Misses         84      84              
  Partials       12      12
Impacted Files Coverage Δ
pkg/cmd/list.go 63.34% <0%> (-1.74%) ⬇️
pkg/cmd/resource_resolver.go 84.72% <100%> (+0.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ab9e06...8e0947a. Read the comment docs.

lizrice
lizrice reviewed Aug 16, 2019
View reviewed changes
Copy link
Contributor

@lizrice lizrice left a comment

Choose a reason for hiding this comment

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

I wonder if this is more complicated than it needs to be? But I could well be wrong...

pkg/cmd/list.go Show resolved Hide resolved
pkg/cmd/list.go Outdated
})
configFlags.AddFlags(cmd.Flags())
o.configFlags.AddFlags(cmd.Flags())
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a simpler fix would have just been to move the lines that were at lines 124-152 to be after these flags have been set up? I don't love the extra complexity of having the separate whoCanOptions struct just to hold the configFlags (but am I missing something?)

@danielpacak
Copy link
Contributor Author

I wonder if this is more complicated than it needs to be? But I could well be wrong...

You were right @lizrice It can be done simpler. Thanks for pointing this out.

lizrice
lizrice approved these changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@lizrice lizrice left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@lizrice lizrice merged commit 6228725 into aquasecurity:master Aug 21, 2019
@danielpacak danielpacak deleted the issue_41_fix_kubeconfig_flag branch August 21, 2019 12:05
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizrice lizrice lizrice approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--kubeconfig doesn't work
3 participants
@danielpacak @codecov-io @lizrice