-
Notifications
You must be signed in to change notification settings - Fork 147
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(k3d): refactor suggestion #2165
Conversation
de5d811
to
03a2b0b
Compare
@FalcoSuessgott the PR is still in draft, but do you want us to review and validate the refactoring idea now? |
HI @fharper, yes its ready for review, I added some simple tests illustrating the purpose of some changes |
@FalcoSuessgott: sorry for the delay, I asked the engineering team again to give it a closer look as soon as possible. |
Merged already😳 I hoped someone tested this and was able to verify the changes :D Nevertheless Im available and can fix if something shows up |
@FalcoSuessgott oh weird, my mistake, not sure what happened as I just wanted to merge main into your main branch to be sure people have the latest when reviewing it. I'll remove the merge on |
Oh no, it's fine, nothing was merged in kubefirst As a side note, it's part of why I always suggest that folks create a branch in their fork before doing a PR, so the verbose and actions are clearer in the GitHub interface (will add this to the contributions guidelines) :) |
@FalcoSuessgott I'm so sorry about the delay, we usually respond a lot faster to PR. @jarededwards told me he will review it as soon as possible. |
Hey @FalcoSuessgott , sorry for the slowness here. I am unable to run your branch. can you please get this in a working state before we review? it hangs with this log line`[PRECHECKS] Running prechecks (skip with KUBEFIRST_DISABLE_PRECHECKS)` thanks in advance! |
Hi @jarededwards, hm bubbletea seemed to have blocked the cobra execution. I've put them both in goroutines and listened for the first error that occurred and exited the program in that case. Maybe give it a try |
I was able to go further with the latest fix. I got an @jarededwards can you give it another try on your side please? |
Hey @FalcoSuessgott [ERROR] error executing command: NGROK_AUTHTOKEN not set, but required (see https://docs.kubefirst.io/k3d/quick-start/install#local-atlantis-executions-optional). Just want to call out that |
@FalcoSuessgott I'm so sorry we let you down with this PR. Since the acquisition, we were all swamped to get back to business as usual. The good news is that our team is growing, and kubefirst will be back on track really soon, so we'll better manage external pull requests like yours. With that said, there no excuses I can give that justify waiting two months to properly start to test and review your pull request. Thanks again for the help, and so sorry for how we handled things: we are still learning as a team. |
Disclaimer:
I was playing around with
kubefirst
andk3d
on GitHub and was checking out the code, since I'm currently traveling and looking for some OSS projects to contribute. The PR turned out to be way bigger than anticipated... I understand in case you guys don't want to invest the time in reviewing. I could also split up and open up multiple PRs containing the changes described next, in case some changes are of interest. I am also willing to expand these changes to the other supported cloud provider.I want to suggest some alternative approaches when building CLI apps with
cobra
, I choosek3d
as an example, since I am able to fully e2e test it locally.The changes can be summarized into:
prechecks
the
k3d create
now has a precheck suite, where certain assertions can be made (env var exists, command is available, ...) and eventually error and thus inform the user early on about missing dependencies, before creating any resources. This can be easily tested and expanded for other prechecks.decluttered the main function
I was able to declutter the main function to more idomatic way and moved the bubbletea and logging initialiaztion in
cobra.OnInitialize()
, as well as the bubbletea quitting in thePostRun()
:split up the
k3d create
logic in functionsThe biggest change might be the split up of the
runK3dCreate
function into functions. Where possible, I avoided if/else for more readability. To do so, I added a struct that contains all required options and sane defaults for creating the k3d cluster. I believe this way the code is more readable and most important testable, since I haven't found any tests so far.. I think some refactors in the underlying libs (kubefirst/runtime, kubefrist/kubefirst-api etc) might be worth itThis allows us to eadily call functions based on the current gitprovider:
other smaller changes
TF_CLI_ARGS="-no-color"
to tf apply runs for having no color encodings in the logfilekubefirst k3d
command by passing custom flags and envs