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.

Sign up for GitHub

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

Jump to bottom

watchNamespaces improvements #1392

Merged
merged 3 commits into from
Jul 25, 2023
Merged

watchNamespaces improvements #1392

merged 3 commits into from
Jul 25, 2023

Conversation

pepov
Copy link
Member

@pepov pepov commented Jul 24, 2023 •
edited
Loading

  • Make logging.spec.watchNamespaces (explicit static list) and logging.spec.watchNamespaceSelector (label based dynamic list) additive instead of exclusive for hopefully better semantics.
  • Add tests for better coverage (looking into reusing the same thing for a different use case as well)
  • Add namespace watches and trigger reconcile on loggings with watchNamespaceSelector defined

Signed-off-by: Peter Wilcsinszky [email protected]

@pepov
watchNamespaces: make the explicit and the selector based config addi…
931dbbf 
…tive instead of mutually exclusive

Signed-off-by: Peter Wilcsinszky <[email protected]>
@pepov
Copy link
Member Author

pepov commented Jul 24, 2023

@nak0f as we are preparing to build on top of these selectors I wanted to reconsider making the two different namespace lists additive instead of exclusive to reduce frustration of users as they have to debug through the logging resource.

also this adds a few tests to cover the behaviour

@pepov pepov changed the title watchNamespaces: make the explicit and the selector based config additive instead of mutually exclusive watchNamespaces improvements Jul 24, 2023
@aslafy-z
Copy link
Collaborator

aslafy-z commented Jul 24, 2023 •
edited
Loading

Another option would be to remove completely watchNamespaces and encourage users to use the kubernetes.io/metadata.name label (available since 1.22) to select by name. In this case, we may want to allow a list of selectors for watchNamespaceSelector instead of a single selector.

@pepov
Copy link
Member Author

pepov commented Jul 25, 2023

@aslafy-z sounds good (I wasn't aware of that label), but that would be a breaking change affecting lots of users, thus I would avoid it now.

@aslafy-z aslafy-z mentioned this pull request Jul 25, 2023
Closed
@aslafy-z
Copy link
Collaborator

I opened #1394 to track this enhancement proposal.

@pepov pepov requested review from siliconbrain and ahma July 25, 2023 09:01
siliconbrain
siliconbrain approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@siliconbrain siliconbrain left a comment

Choose a reason for hiding this comment

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

lgtm

@pepov pepov merged commit a98426a into master Jul 25, 2023
@pepov pepov deleted the watch-namespaces-1 branch July 25, 2023 15:42
nak0f pushed a commit to nak0f/logging-operator that referenced this pull request Nov 22, 2023
@pepov @nak0f
watchNamespaces improvements (kube-logging#1392)
2b24de4 
Make logging.spec.watchNamespaces (explicit static list) and logging.spec.watchNamespaceSelector (label based dynamic list) additive instead of exclusive for hopefully better semantics.
Add tests for better coverage (looking into reusing the same thing for a different use case as well)
Add namespace watches and trigger reconcile on loggings with watchNamespaceSelector defined

Signed-off-by: Peter Wilcsinszky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siliconbrain siliconbrain siliconbrain approved these changes

@ahma ahma Awaiting requested review from ahma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pepov @aslafy-z @siliconbrain