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 ConfigMap collector namespace-optional #1212

Merged
merged 6 commits into from
Jun 13, 2023

Conversation

adamancini
Copy link
Member

@adamancini adamancini commented Jun 6, 2023

Description, Motivation and Context

When no namespace is given in the ConfigMaps collector spec, use the namespace from the current kubectl context.

Fixes: #907

Checklist

  • New and existing tests pass locally with introduced changes.
  • Tests for the changes have been added (for bug fixes / features)
  • The commit message(s) are informative and highlight any breaking changes
  • Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

  • Yes
  • No

@adamancini adamancini changed the title Configmap collector make ConfigMap collecto namespace-optional Jun 6, 2023
@adamancini adamancini changed the title make ConfigMap collecto namespace-optional make ConfigMap collector namespace-optional Jun 6, 2023
@adamancini adamancini added the type::bug Something isn't working label Jun 6, 2023
@adamancini adamancini marked this pull request as ready for review June 6, 2023 14:33
@adamancini adamancini requested a review from a team as a code owner June 6, 2023 14:33
@adamancini
Copy link
Member Author

Using the test spec from my PR:

tail support-bundle-2023-06-06T10_09_37/configmaps/* support-bundle-2023-06-06T10_09_37/configmaps-errors/* 
==> support-bundle-2023-06-06T10_09_37/configmaps/cluster-info.json <==
{
  "namespace": "",
  "name": "cluster-info",
  "key": "",
  "configMapExists": false,
  "keyExists": false,
  "data": null
}
==> support-bundle-2023-06-06T10_09_37/configmaps/does-not-exist.json <==
{
  "namespace": "",
  "name": "does-not-exist",
  "key": "",
  "configMapExists": false,
  "keyExists": false,
  "data": null
}
==> support-bundle-2023-06-06T10_09_37/configmaps/kube-proxy.json <==
{
  "namespace": "",
  "name": "kube-proxy",
  "key": "",
  "configMapExists": true,
  "keyExists": false,
  "data": null
}
==> support-bundle-2023-06-06T10_09_37/configmaps/kube-root-ca.crt.json <==
{
  "namespace": "",
  "name": "kube-root-ca.crt",
  "key": "",
  "configMapExists": true,
  "keyExists": false,
  "data": null
}
==> support-bundle-2023-06-06T10_09_37/configmaps/kube-system <==

==> support-bundle-2023-06-06T10_09_37/configmaps-errors/cluster-info.json <==
[
  "configmaps \"cluster-info\" not found"
]
==> support-bundle-2023-06-06T10_09_37/configmaps-errors/does-not-exist.json <==
[
  "configmaps \"does-not-exist\" not found"
]%                                                                                                   

Copy link
Member

@diamonwiggins diamonwiggins left a comment

Choose a reason for hiding this comment

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

Approved with a suggestion, and a caveat of there has been some discussion recently if we should ever use the kubeconfig context's namespace at all. Not sure what the argument is there, but i don't object to this change.

pkg/collect/configmap.go Outdated Show resolved Hide resolved
diamonwiggins
diamonwiggins previously approved these changes Jun 6, 2023
Copy link
Member

@diamonwiggins diamonwiggins left a comment

Choose a reason for hiding this comment

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

Approved with a suggestion, and a caveat of there has been some discussion recently if we should ever use the kubeconfig context's namespace at all. Not sure what the argument is there, but i don't object to this change.

adamancini and others added 2 commits June 12, 2023 16:05
Co-authored-by: Diamon Wiggins <38189728+diamonwiggins@users.noreply.github.com>
@adamancini adamancini merged commit 03c53ca into replicatedhq:main Jun 13, 2023
21 checks passed
@adamancini adamancini deleted the configmap-collector branch June 13, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug::normal type::bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConfigMap collector should be namespace optional
3 participants