-
Notifications
You must be signed in to change notification settings - Fork 415
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 pipeline version to v0.16.3 #767
Conversation
The following is the coverage report on the affected files.
|
55fe116
to
c3bcf09
Compare
The following is the coverage report on the affected files.
|
c3bcf09
to
c3a4449
Compare
The following is the coverage report on the affected files.
|
c3a4449
to
aa86354
Compare
The following is the coverage report on the affected files.
|
@dibyom PR is ready to review |
} | ||
|
||
// GetConditionSet retrieves the condition set for this resource. Implements | ||
// the KRShaped interface. |
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.
Question: Do we have to implement the KRShaped interface as part of this PR? I think this adds a new status.Condition
of type Ready
-- which is what we'd want but we'd also have to add logic for when Ready
is true (e.g. when deployment/service are ready).
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.
Pipeline seems to disable it for the existing types at the moment: https://github.com/tektoncd/pipeline/blob/master/pkg/apis/pipeline/v1beta1/pipelinerun_types.go#L41
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.
yes a new condition of type Ready
needed
and i have handled here https://github.com/tektoncd/triggers/pull/767/files#diff-ac6d17e220f6c870c0bc0304bc7c53b4R115-R117
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.
If we want to do it same like pipeline we can disable it no issues
Let me know
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.
Yeah, so if we are adding Ready we should ensure that it is only true iff both deployment and service are also ready. I think this will be a useful follow up. But since that's a feature and adds new fields to the type, we can add it in a follow up?
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.
Ya that make sense 👍
aa86354
to
0c3e82a
Compare
The following is the coverage report on the affected files.
|
Thanks @savitaashture Could we also update the Release Notes to mention the updated minimum version of k8s needed? |
0c3e82a
to
7414bc1
Compare
/retest |
e85b885
to
37fbc79
Compare
37fbc79
to
894867c
Compare
Updated Release Notes |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom 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 |
/kind misc |
Not 100% sure, but we might also be running into tektoncd/pipeline#3237 with this change. Might be worth calling out in the release notes unless we get to upgrade again to a newer knative/pkg that has a fix. |
Changes
Bumped to pipeline version v0.16.3
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.
Release Notes
The manual changes are:
Promote
in tests, otherwise our generated reconcilers won't do anything (they aren't the leader!)