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

feat: safer context parsing, general refactoring #228

Merged
merged 7 commits into from
Mar 10, 2020
Merged

feat: safer context parsing, general refactoring #228

merged 7 commits into from
Mar 10, 2020

Conversation

sh0rez
Copy link
Member

@sh0rez sh0rez commented Feb 29, 2020

Did some housekeeping in pkg/kubernetes/client, replaced most panic situations with proper errors that include context around the cause.

This should result in more safe parsing of kubectl config view, so that errors from there are not as ugly as before.

Also did some general refactoring, linter fixes, etc

/cc @cyriltovena @mplzik

@sh0rez sh0rez added the kind/enhancement Improve something existing label Feb 29, 2020
@sh0rez sh0rez requested a review from rfratto February 29, 2020 20:39
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

I have a couple of nits but this LGTM.

pkg/defers/defers.go Outdated Show resolved Hide resolved
pkg/kubernetes/client/context.go Outdated Show resolved Hide resolved
pkg/kubernetes/client/context.go Outdated Show resolved Hide resolved
Previously, context parsing used a lot of `objx.Must`, which can panic.
Also, schemas were not properly resolved, instead just unmarshalled onto
`map[string]interface`, so possible errors were not caught.

Now uses very explicit error handling so that it does not panic anymore,
but instead print proper (and hopefully helpful) error messages
Adds global defer using a custom package so that we can schedule tasks
for run on exit.

This is used for removing the environment patch from the temp directory
after each run, so we don't leak possibly sensitive data there
Improves code style by fixing the most important linter
discoveries (golint, govet, golangci-lint)
The native funcs were `var` before, because functions can't be `const`.
Variable native funcs made so sense however, so we now use functions
returning the native func, so they are effectively constant
Global defers are probably not the best idea and as we can avoid it, we
should.

Switches the cleanup required for the kubectl nsPatch to the `io.Closer`
pattern.
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM!

@sh0rez sh0rez merged commit 9fa4f27 into master Mar 10, 2020
@sh0rez sh0rez deleted the safe-ctx branch March 10, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improve something existing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants