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

[plugins/k8s][feat] Add argument to collect all contexts in config file #727

Merged
merged 1 commit into from Mar 24, 2022

Conversation

tdickers
Copy link
Contributor

Description

This avoids the need to specify every context in order to have it imported.

Added a new argument rather than a magic value like all for --k8s-context to avoid any potential conflicts.

To-Dos

  • Add test coverage for new or updated functionality (to the extent that there's existing coverage to extend)
  • Lint and test with tox
  • N/A Document new or updated functionality (someengineering/resoto.com#XXXX)

Code of Conduct

By submitting this pull request, I agree to follow the code of conduct.

This avoids the need to specify every context in order to have it imported.
@tdickers
Copy link
Contributor Author

tdickers commented Mar 23, 2022

Example output from testing (docker-compose running resotocore, etc., running resotoworker locally from venv):

GOOGLE_APPLICATION_CREDENTIALS="$HOME/.config/gcloud/application_default_credentials.json" resotoworker --k8s-all-contexts --k8s-config "$HOME/.kube/config.something" --collector k8s --psk changeme --k8s-collect nodes -v

22-03-23 17:59:56|resotoworker|DEBUG|21334|collector_k8s  Starting new collect process for k8s
22-03-23 17:59:56|resotoworker|DEBUG|21334|       k8s  plugin: Kubernetes collecting resources
22-03-23 17:59:57|resotoworker|DEBUG|21334|       k8s  importing all contexts in configuration file since --k8s-all-contexts was specified
22-03-23 17:59:57|resotoworker|DEBUG|21334|       k8s  loading context somecontext

@MrMarvin
Copy link
Contributor

Great idea, will make loading an entire fleet of clusters much more convenient. 👍

However I believe this will clash with the planned config / parameter rework for the 2.0.0 release. @lloesche do you think we could get this in before or do should we target post release?

@lloesche
Copy link
Member

Thank you for the PR, looks good to me. We can merge it now but like @MrMarvin said, once we merge the breaking config change (will happen this weekend or around the beginning of next week) to main almost all CLI args will disappear and be replaced by config options of the same name (we realised how ridiculous it is to have 240 lines of --help output 😄).

So instead of --k8s-all-contexts it'll be a config option like

k8s:
  all_contexts: false

Which can be edited using > config edit resotoworker, loaded from a file or alternatively --override k8s.all_contexts=false or the equivalent ENV format ala RESOTOWORKER_OVERRIDE: k8s.all_contexts=false.

In plugin code in the future it'll be Config.k8s.all_contexts instead of ArgumentParser.args.k8s_all_contexts.

@lloesche lloesche marked this pull request as ready for review March 24, 2022 12:21
@lloesche lloesche merged commit 7556ac2 into someengineering:main Mar 24, 2022
@lloesche
Copy link
Member

Merging this since it makes a lot of sense for k8s and we're just in the process of cutting the 2.0.0a19 release which includes a hotfix for load balancer collection.

@tdickers
Copy link
Contributor Author

Thanks!

@tdickers tdickers deleted the k8s-allow-all-contexts branch March 24, 2022 14:13
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.

None yet

3 participants