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

fix(#4556): disable creating k8s client in output mode #4557

Merged
merged 1 commit into from
Jul 12, 2023
Merged

fix(#4556): disable creating k8s client in output mode #4557

merged 1 commit into from
Jul 12, 2023

Conversation

anhpngt
Copy link
Contributor

@anhpngt anhpngt commented Jul 10, 2023

This is my attempt to fix #4556. The approach includes:

  • For run: move cmd.Annotations[offlineCommandLabel] = "true" to PersistentPreRunE so that the command got marked as offline earlier in the run. bind doesn't require this change.
  • Run GetCmdClient() only when NOT in offline mode
  • Since we don't have any k8s clients anymore, some values are hard-coded, such as scheme.Scheme for printers.NewTypeSetter() function.
  • Add unit tests to cover these cases in the future

Fixes: #4556

Release Note

fixes(cli): kamel run no longer requires connecting to k8s cluster in output mode

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Thanks, good point on spotting the root cause. Please, double check also the bind and the promote command which can be affected. I think that we can introduce the following unit test as well to make sure all is covered (adding a bogus kube config will make sure that we don't use any one available, reason why it is not discovered now):

$ touch /tmp/test.cfg
$ kamel run /tmp/Test.java -o yaml --kube-config /tmp/test.cfg 
Error: invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable

Same stuff for bind and promote, have a look at the related unit test.

@squakez squakez added the kind/bug Something isn't working label Jul 11, 2023
@anhpngt
Copy link
Contributor Author

anhpngt commented Jul 11, 2023

Thank you for the feedback. I think only run and bind are affected by this bug. promote cannot be run without an actual operator, so I don't think we can do anything about it. Let me further fix for bind and have the unit tests ready for both.

There are other offline commands, version and help, but they are working fine, so I guess let's leave them be.

This fixes `bind` and `run` commands.
@anhpngt
Copy link
Contributor Author

anhpngt commented Jul 12, 2023

hi @squakez, I updated the fix for bind and added unit tests for both commands. I tried running the unit test without the fix, it correctly detected the errors

$ go test -run TestBindOutputWithoutKubernetesCluster
--- FAIL: TestBindOutputWithoutKubernetesCluster (0.00s)
    bind_test.go:261: 
                Error Trace:    /home/anhpngt/apache/camel-k/pkg/cmd/bind_test.go:261
                Error:          Received unexpected error:
                                invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable
                Test:           TestBindOutputWithoutKubernetesCluster
FAIL
exit status 1
FAIL    github.com/apache/camel-k/v2/pkg/cmd    0.024s

$ go test -run TestRunOutputWithoutKubernetesCluster
--- FAIL: TestRunOutputWithoutKubernetesCluster (0.00s)
    run_test.go:830: 
                Error Trace:    /home/anhpngt/apache/camel-k/pkg/cmd/run_test.go:830
                Error:          Received unexpected error:
                                invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable
                Test:           TestRunOutputWithoutKubernetesCluster
FAIL
exit status 1
FAIL    github.com/apache/camel-k/v2/pkg/cmd    0.026s

@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ✔️ - Coverage unchanged.

@squakez squakez merged commit 84b43e8 into apache:main Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kamel run in output mode still requires connecting to a k8s cluster
2 participants