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

scope kubernetes discovery to a single namespace #223

Merged
merged 3 commits into from
Oct 26, 2018

Conversation

edaniszewski
Copy link
Contributor

fixes #222

This also adds in a basic test case for the config to make sure that no kubernetes config values get set by default. We only want discovery to happen if its set explicitly.

tested this on a gke cluster w/ synse-server and emulator plugin.

@codecov
Copy link

codecov bot commented Oct 26, 2018

Codecov Report

Merging #223 into master will decrease coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   87.55%   87.52%   -0.03%     
==========================================
  Files          46       46              
  Lines        1358     1363       +5     
==========================================
+ Hits         1189     1193       +4     
- Misses        169      170       +1
Impacted Files Coverage Δ
synse/config.py 100% <ø> (ø) ⬆️
synse/discovery/kubernetes.py 98.59% <88.88%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 695a382...c999a2a. Read the comment docs.

Copy link
Contributor

@marcoceppi marcoceppi left a comment

Choose a reason for hiding this comment

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

LGTM with a quick fix for simplicity

# If no namespace is provided via user configuration, if will default to
# the 'default' namespace.
ns = config.options.get('plugin.discover.kubernetes.namespace')
if not ns:
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved to the previous line as Dict.get supports default values

@edaniszewski edaniszewski merged commit ac87f8a into master Oct 26, 2018
@edaniszewski edaniszewski deleted the discovery-namespaced branch October 26, 2018 14:53
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.

Scope kubernetes lookups to a single namespace
2 participants