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

Make the kubeseal CLI have a skinny main #939

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

josvazg
Copy link
Collaborator

@josvazg josvazg commented Aug 30, 2022

Description of the change

The CLI had way too much code within the main package.

This change moves most of the kubeseal CLI code to a package "pkg/kubeseal", leaving a simple "main" which is only concerned with CLI flags and other command line perks.

Note that the changes are minimal, and only functions affected by the split of code between main and the package are really affected. The rest is code copied to another package.

Other changes included:

  • Flag fields needed to become public, and this are upper case now.
  • There is a new NewConfig() function now.
  • VERSION handling is handled by main completely and is out of the Flags struct now.

Benefits

Code in the new pkg is accesible from other packages and can be further refactored and improved.

Possible drawbacks

This is still a mostly mechanical code move, it does not yet address making the kubeseal code look and feel like a proper library to be consumed by code outside of a CLI context. This was left out of scope intentionally to make progress on smaller incremental steps.

Additional information

Once this code is merged, the intent going forward is to start refactoring things out of the big Run function in small steps. The expectation of the final result is that a smaller (and skinny) version of such "run" might go back to the main code but it will only decide what operations or behaviors of kubeseal will be invoked by each CLI execution. Many of those behaviors or operations will become new functions within kubeseal. Hopefully we can get rid of using the Flags struct within kubeseal.

@josvazg
Copy link
Collaborator Author

josvazg commented Aug 30, 2022

Tip: The diff might be more confusing that actually looking at the resulted main.go and main_test.go now to have a sense of what has remained there. The "pkg/kubeseal" code is mostly copy& paste from main except for those bits that could not be moved.

See main final state as a whole here:

alvneiayu
alvneiayu previously approved these changes Aug 31, 2022
Copy link
Collaborator

@alvneiayu alvneiayu left a comment

Choose a reason for hiding this comment

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

LGTM

@josvazg
Copy link
Collaborator Author

josvazg commented Aug 31, 2022

I found a weird bug on the refactor and it ended up being because goimports had changed the imports in the moved code and picked the wrong import for scheme, choosing the local package over a dependency ("k8s.io/client-go/kubernetes/scheme")

In order to detect other instances of goimports making wrong choices, I did a comparison of old and new imports. I expect that new imports on the main AND kubeseal be the same as the old main. Same for tests, new test files combined should import the same things as the old main_test code did.

$ cat /tmp/imports-new-main.txt /tmp/imports-new-kubeseal.txt | sort -u | grep -v '^$' > /tmp/new-code-imports
$ sort -u < /tmp/imports-old-main.txt |grep -v '^$' > /tmp/expected-code-imports
$ diff /tmp/expected-code-imports /tmp/new-code-imports 
13a14
> 	"github.com/bitnami-labs/sealed-secrets/pkg/kubeseal"
28c29
< 	"k8s.io/klog/v2"
---
> 	"k8s.io/klog"

This happened because goimports knew it needed to import a klog package ,but it did not know it had to be version 2.

Note the github.com/bitnami-labs/sealed-secrets/pkg/kubeseal is expected on the new main and not the old, that is the code we moved away.

Test code showed no differences, so we are fine there:

$ cat /tmp/imports-new-main_test.txt /tmp/imports-new-kubeseal_test.txt | sort -u | grep -v '^$' > /tmp/new-test-imports
$ sort -u < /tmp/imports-old-main_test.txt |grep -v '^$' > /tmp/expected-test-imports
$ diff /tmp/expected-test-imports /tmp/new-test-imports 
$ 

Fix for klog/v2 coming up...

Copy link
Collaborator

@alvneiayu alvneiayu left a comment

Choose a reason for hiding this comment

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

LGTM

@josvazg josvazg merged commit 1a59a30 into bitnami-labs:main Sep 2, 2022
This was referenced Sep 22, 2022
@josvazg josvazg deleted the make-cli-main-skinny branch October 11, 2022 17:06
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