-
Notifications
You must be signed in to change notification settings - Fork 4.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
Add event ttl flag #7487
Add event ttl flag #7487
Conversation
Welcome @tioxy! |
Hi @tioxy. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Could you explain why we have that big changes in pkg/apis/kops/types.generated.go and protokube/pkg/gossip/mesh/mesh.pb.go otherwise this PR looks good |
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
30be1fa
to
2840a63
Compare
Sure @zetaab The changes inside make apimachinery
# This line below is commented inside "apimachinery-codegen" function in Makefile
# There is a comment "codecgen works only if invoked from directory where the file is located"
# I thought it was needed to generated code, so I ran it
cd pkg/apis/kops/ && ~/k8s/bin/codecgen -d 1234 -o types.generated.go instancegroup.go cluster.go
make The changes inside make apimachinery
make I pushed changes to the codegen commit removing unnecessary changes 😁 |
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.
/ok-to-test
Now it looks good! Thanks
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.
Hi @tioxy this looks good, just requesting 1 change before we look at merging it in.
Hi @rdrgmnzs, I made some research around It seems that Currently, our Decode function for codecs is also built on top of apimachinery packages(runtime and serializer). I don't think we'll run into compatibility issues anytime soon 😃 |
2840a63
to
49fb81e
Compare
49fb81e
to
a1a3d44
Compare
Fixed merge conflicts |
9d44a7b
to
f429f09
Compare
ffab979
to
ac7e6e4
Compare
ac7e6e4
to
18a2491
Compare
Enable cluster spec to support "event-ttl" flag from kube-apiserver to change event retention time
Add inside flagbuilder a new test case to check if event-ttl flag is being generated properly
Run apimachinery & crds to generate "zz_generated*" files and to update cluster crd
18a2491
to
6b8af27
Compare
Added tests for |
/assign @rdrgmnzs |
Thanks @tioxy /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, tioxy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…-origin-release-1.15 Automated cherry pick of #7487: Add Event TTL flag
What type of PR is this?
/kind api-change
What this PR does / why we need it:
Add support for kops to declare
event-ttl
flag fromkube-apiserver
Which issue(s) this PR fixes:
Fixes #7484
Special notes for your reviewer:
Should I remove
pkg/apis/kops/types.generated.go
from this PR?This file made this PR longer than it should be 😞
I ran this commented command here
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: