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

Fix event publishing for cluster scoped crds #847

Merged
merged 7 commits into from
Mar 8, 2022
Merged

Conversation

zhrebicek
Copy link
Contributor

  • There is no api for publishing cluster scoped events
  • It default to default namespace
  • Added ability to override namespace on Recorder::new

Motivation

We were getting 405 when trying to publish events for cluster scoped crds.

Solution

As there seems to be no endpoint for cluster scoped events.
I have added default namespace default for cluster scoped crds and I have added way to override it for all the cases.

I am not sure about the namespace_override but it seems good enough as is, maybe needs better comment/renaming.

cc: @teozkr

@clux
Copy link
Member

clux commented Mar 8, 2022

There is no api for publishing cluster scoped events

ah thanks for this. TIL.

although, if this is true, then i'm thinking it might be better to fallback to Api::default_namespaced rather than force a string arg on everyone here 🤔

@zhrebicek
Copy link
Contributor Author

zhrebicek commented Mar 8, 2022

although, if this is true, then i'm thinking it might be better to fallback to Api::default_namespaced rather than force a string arg on everyone here 🤔

I will fix the DCO thingy and try to add it there.

@clux should I also drop the namespace_override or would you like to keep it there?

@clux
Copy link
Member

clux commented Mar 8, 2022

@clux should I also drop the namespace_override or would you like to keep it there?

yeah, i don't think we need an override for it. The default namespace is documented as:

The default namespace for objects with no other namespace

and events for a cluster scoped resource fits that definition pretty well.

i'll put in a suggestion.

kube-runtime/src/events.rs Outdated Show resolved Hide resolved
@clux clux added the changelog-fix changelog fix category for prs label Mar 8, 2022
@clux clux added this to the 0.70.0 milestone Mar 8, 2022
zhrebicek and others added 3 commits March 8, 2022 17:47
* There is no api for publishing cluster scoped events
* It default to `default` namespace
* Added ability to override namespace on Recorder::new

Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
Co-authored-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
@zhrebicek zhrebicek requested a review from clux March 8, 2022 16:51
@zhrebicek
Copy link
Contributor Author

I see I have formatted this kube-runtime/src/watcher.rs by mistake, should I leave it as is ?

@clux
Copy link
Member

clux commented Mar 8, 2022

I see I have formatted this kube-runtime/src/watcher.rs by mistake, should I leave it as is ?

The fmt job should not pass if you accidentally formatted something. Note that formatting is done with nightly; rustup update nightly then make fmt to fix it.

Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

minor comment nits, but LGTM

kube-runtime/src/events.rs Outdated Show resolved Hide resolved
kube-runtime/src/events.rs Outdated Show resolved Hide resolved
zhrebicek and others added 2 commits March 8, 2022 23:14
Co-authored-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
Co-authored-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: Zdeněk Hřebíček <zdenek.hrebicek@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #847 (b2109e2) into master (0b5e803) will increase coverage by 0.13%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
+ Coverage   70.31%   70.44%   +0.13%     
==========================================
  Files          59       59              
  Lines        4184     4203      +19     
==========================================
+ Hits         2942     2961      +19     
  Misses       1242     1242              
Impacted Files Coverage Δ
kube-runtime/src/events.rs 94.44% <95.45%> (+1.99%) ⬆️

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 0b5e803...b2109e2. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants