-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
New API group for Events. #49112
New API group for Events. #49112
Conversation
f5fc4e3
to
5ddde7d
Compare
@caesarxuchao - can you please finish this PR, but adding stuff that's needed for As I don't have any idea what needs to be done and it's not documented anywhere I'll need to fall back to just modifying Event type in place in v1.Core group. |
Have you taken a look at the doc updates in kubernetes/community#788? |
No - I've found only last updates from few months back, which weren't merged. |
bd3bba3
to
f2bdfe3
Compare
pkg/apis/events/types.go
Outdated
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
||
// Event is a report of an event somewhere in the cluster. It generally denotes some state change in the system. | ||
type EventTwo struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't call this EventTwo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - that's a placeholder. Current API machinery doesn't allow creating type Event
inside group events
, because it tries to be smart and creates a type events
(plural for type Event
) and we get name collision. Is it likely to get fixed in 1.8 timeframe? @kubernetes/sig-api-machinery-bugs @sttts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmarek do you have a gist with the conflict in code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it the type in the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried patching it, but it didn't help - updated this PR with the result.
@sttts gist for you:
pkg/client/clientset_generated/internalclientset/typed/events/internalversion/event.go:48: events redeclared in this block
Once here:
import (
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
types "k8s.io/apimachinery/pkg/types"
watch "k8s.io/apimachinery/pkg/watch"
rest "k8s.io/client-go/rest"
events "k8s.io/kubernetes/pkg/apis/events"
scheme "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/scheme"
)
and once here:
// events implements EventInterface
type events struct {
client rest.Interface
ns string
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry - I didn't notice that I need to add overrides. Trying it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, still doesn't work.
pkg/apis/events/types.go
Outdated
// Required. Time when this Event was first observed. | ||
EventTime metav1.MicroTime | ||
// Optional. Data about the Event series this event represents or nil if it's a singleton Event. | ||
Series *EventSeries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kensimon speak now or forever hold your peace.
9e212e3
to
4f385da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as-is, but a few nits
@@ -25,8 +25,8 @@ const ( | |||
PodStatusField = "status.phase" | |||
SecretTypeField = "type" | |||
|
|||
EventReasonField = "reason" | |||
EventSourceField = "source" | |||
EventReasonField = "action" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what these are for - nothing seems to use them in the codebase..
|
||
// Name of the controller that emitted this Event, e.g. `kubernetes.io/kubelet`. | ||
// +optional | ||
ReportingController string `json:"reportingController,omitempty" protobuf:"bytes,4,opt,name=reportingController"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All optional fields should be pointers
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, gmarek, thockin Associated issue: 383 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[MILESTONENOTIFIER] Milestone Pull Request Current @caesarxuchao @derekwaynecarr @gmarek @smarterclayton @sttts @thockin Note: This pull request is marked as Example update:
Pull Request Labels
|
/retest Review the full test history for this PR. |
@gmarek: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 55952, 49112, 55450, 56178, 56151). If you want to cherry-pick this change to another branch, please follow the instructions here. |
Fix kubernetes/enhancements#383
cc @shyamjvs