-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Bump knative to release-0.20 #3605
Conversation
/cc @tektoncd/core-maintainers |
a5bfcb2
to
e795a6a
Compare
Note that this bump the minimum required k8s to 1.17 (https:/knative/pkg/blob/release-0.19/version/version.go) /hold |
Also note that the integration test fails because of this min version, because the /cc @mattmoor |
e795a6a
to
7da84a2
Compare
7da84a2
to
fafc0f4
Compare
Signed-off-by: Vincent Demeester <[email protected]>
fafc0f4
to
882a4a7
Compare
This most likely fixes #3663 too 🙃 |
Do not call flag.Parse() as it happens in `sharedmain.ParseAndGetConfigOrDie`. Move anything that needs the flag value after that call. This also does something with QPS and Burst flag are they are already defined in knative/pkg *but* with different default. Signed-off-by: Vincent Demeester <[email protected]>
93f1bc5
to
87b1059
Compare
/hold cancel |
if cfg.QPS == 0 { | ||
cfg.QPS = 2 * rest.DefaultQPS | ||
} | ||
if cfg.Burst == 0 { | ||
cfg.Burst = rest.DefaultBurst | ||
} | ||
// FIXME(vdemeester): this is here to not break current behavior | ||
// multiply by 2, no of controllers being created | ||
cfg.QPS = 2 * float32(*qps) | ||
cfg.Burst = 2 * *burst | ||
cfg.QPS = 2 * cfg.QPS | ||
cfg.Burst = 2 * cfg.Burst |
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.
@tektoncd/core-maintainers This is a bit weird but it's there to keep the same behavior as of today. Because the flag default value is defined in knative/pkg, we don't have the control over it, so we detect if the default value are passed or not. The 2 * …
I am not sure about why we are doing this, but I didn't want to change the behavior here.
@vdemeester what flag handling does this fix? |
I linked the issue in comments. We do call |
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.
Thank you for this! Let's try and get this into v0.20.0
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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 |
/test check-pr-has-kind-label |
/kind check-pr-has-kind-label |
@vdemeester: Thanks for fixing the flag issue. :) |
@@ -142,7 +142,7 @@ func getPipelineRunController(t *testing.T, d test.Data) (test.Assets, func()) { | |||
ctx, cancel := context.WithCancel(ctx) | |||
ensureConfigurationConfigMapsExist(&d) | |||
c, informers := test.SeedTestData(t, ctx, d) | |||
configMapWatcher := configmap.NewInformedWatcher(c.Kube, system.GetNamespace()) | |||
configMapWatcher := cminformer.NewInformedWatcher(c.Kube, system.GetNamespace()) |
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.
The point of this split btw was to enable you to write tests like this for your tiny containers:
https:/mattmoor/mink/blob/master/cmd/extract-digest/depcheck_test.go#L27
Not necessary for this change, but we should follow up to add checks for all the containers that should be nice and tiny.
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.
/lgtm
Registry 503:
/retest |
Looks like it maybe timing related:
I don't see anything else obvious in the logs, but y'all don't seem to describe the pods, which is generally pretty useful. /retest |
Changes
Bump knative to 0.20.
This also fix controller flag handling 🙃 (#3663, maybe others) :
Do not call flag.Parse() as it happens in
sharedmain.ParseAndGetConfigOrDie
. Move anything that needs the flagvalue after that call.
This also does something with QPS and Burst flag are they are already
defined in knative/pkg but with different default.
/kind misc
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes