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(cli): Add a config command to manage the default settings #3599

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented Sep 5, 2022

fixes #1184

Motivation

If we don't want to execute the commands against the default namespace, we need to add the flag -n to all commands or use the command kubectl config set-context --current --namespace=<my-namespace> to set the default namespace at kubectl config level, but we would like to propose something out of the box in kamel cli

Modifications:

  • Add a new command called config to set the default namespace and list the existing settings
  • Allow to read and set the settings in the 4 supported locations
  • Ensure that the namespace used is the one set in the configuration of the config command if any when no namespace has been specified in the command
  • Add some doc about how to use it
  • Fix the existing doc file-based-config.adoc

Release Note

feat(cli): Add a config command to manage the default settings

Copy link
Member

@tadayosi tadayosi left a comment

Choose a reason for hiding this comment

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

That's an awesome enhancement! To understand how this cmd intends to be used, could you provide some example usages? It'd be also great if you could add a section for the usages of the cmd here:
https://github.com/apache/camel-k/blob/main/docs/modules/ROOT/pages/cli/file-based-config.adoc

pkg/cmd/config.go Outdated Show resolved Hide resolved
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.

Nice work. However, some example on how to use the new command would be good to see how it works. Either as a documentation or as an additional comment to this PR.

@essobedo essobedo force-pushed the 1184/configure-default-namespace branch 2 times, most recently from 214ae9a to 6a6a3a3 Compare September 5, 2022 10:15
@essobedo
Copy link
Contributor Author

essobedo commented Sep 5, 2022

I've just added some doc hoping that it clarifies it a little bit more

@essobedo essobedo requested review from tadayosi and squakez September 5, 2022 10:17
@essobedo essobedo force-pushed the 1184/configure-default-namespace branch 2 times, most recently from ae2500e to 172e8f1 Compare September 5, 2022 10:34
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.

LGTM. I'd make an additional effort to include an E2E test to make sure we won't break this feature in the future.

@essobedo
Copy link
Contributor Author

essobedo commented Sep 5, 2022

LGTM. I'd make an additional effort to include an E2E test to make sure we won't break this feature in the future.

Do you mean that E2E tests are also expected even for pure client-side features? I mean the operator is not involved and it only manipulates local files on the client side that is why I proposed only unit tests. I have a test that checks the behavior too https://github.com/apache/camel-k/pull/3599/files#diff-4e0df340b8c06d6aa9b35d150ac8f92bd3581782cbe423209151170311ccbc6dR120-R137, do you still prefer to have an additional E2E test?

@squakez
Copy link
Contributor

squakez commented Sep 5, 2022

@essobedo yeah, I think we can test this as well. I think we can have this configuration file with default namespace created ad hoc from the test (instead of using the -n as we're doing for the other tests), then running an integration and verifying that this is running.

@tadayosi
Copy link
Member

tadayosi commented Sep 6, 2022

For E2E, this place should be ideal to put some config_test.go:
https://github.com/apache/camel-k/tree/main/e2e/namespace/install/cli

@essobedo essobedo force-pushed the 1184/configure-default-namespace branch from 172e8f1 to e05498e Compare September 6, 2022 08:10
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.

Great, looks good!

e2e/namespace/install/cli/config_test.go Show resolved Hide resolved
e2e/namespace/install/cli/config_test.go Outdated Show resolved Hide resolved
e2e/namespace/install/cli/config_test.go Show resolved Hide resolved
@essobedo essobedo force-pushed the 1184/configure-default-namespace branch from e05498e to 123190c Compare September 6, 2022 10:32
@essobedo essobedo requested a review from tadayosi September 6, 2022 14:32
@essobedo essobedo force-pushed the 1184/configure-default-namespace branch from 123190c to dd50d80 Compare September 7, 2022 07:36
@squakez squakez merged commit 90888bc into main Sep 7, 2022
@squakez squakez deleted the 1184/configure-default-namespace branch September 7, 2022 12:45
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.

kamel CLI - Allow to specify a namespace to use instead of default
4 participants