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

Update Sequence to use correct conditions. #5369

Merged
merged 4 commits into from
May 11, 2021

Conversation

n3wscott
Copy link
Contributor

@n3wscott n3wscott commented May 7, 2021

Fixes #4929

  • Fixed logic bug when setting sequence to a nil address
  • Use ready condition of channel and not the addressable state of channels.
  • Update conditions from False to Unknown when setting non-terminal conditions.

/kind cleanup

Sequence now displays conditions as `Unknown` rather than `False` for non-terminal condition states.

@knative-prow-robot knative-prow-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 7, 2021
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label May 7, 2021
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 7, 2021
@n3wscott n3wscott force-pushed the sequences-api-v1 branch from 820b051 to 122c7f8 Compare May 7, 2021 17:56
@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #5369 (ee00ab2) into main (f1f4615) will decrease coverage by 0.04%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5369      +/-   ##
==========================================
- Coverage   82.65%   82.61%   -0.05%     
==========================================
  Files         194      194              
  Lines        6002     6004       +2     
==========================================
- Hits         4961     4960       -1     
- Misses        717      720       +3     
  Partials      324      324              
Impacted Files Coverage Δ
pkg/apis/flows/v1/sequence_types.go 100.00% <ø> (ø)
pkg/apis/flows/v1/sequence_lifecycle.go 91.80% <84.61%> (-4.81%) ⬇️

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 f1f4615...ee00ab2. Read the comment docs.

@n3wscott n3wscott requested a review from lionelvillard May 7, 2021 18:07
go.sum Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/flows/v1/sequence_lifecycle.go 95.5% 91.3% -4.2

@@ -124,10 +124,11 @@ type SequenceStatus struct {
// Matches the Spec.Steps array in the order.
ChannelStatuses []SequenceChannelStatus `json:"channelStatuses"`

// AddressStatus is the starting point to this Sequence. Sending to this
// Address is the starting point to this Sequence. Sending to this
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand correctly. This is equivalent because we inlined AddressStatus before and instead of inlining you changed it to be explicit? Is there a reason why this is not a pointer like what AddressStatus has?

https://github.com/knative/pkg/blob/main/apis/duck/v1/addressable_types.go#L64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am doing what Broker does, we have both styles and because the inner url is a pointer, it seems like the wrapper to the addressable could be allowed to be not a pointer?

We should pick a style and make it consistent. The root issue might be https://github.com/knative/pkg/blob/main/apis/duck/v1/addressable_types.go#L64 has URL as not optional

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, should've been clearer. I saw there being inconsistencies also. But just wanted to make sure that my understanding is that there are no changes to wire format. We should probably vet these to make sure they are consistent, and make sure that we comment the duck properly since it says there to embed it like this (well, that's how I read it still).

Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2021
@knative-prow-robot knative-prow-robot merged commit b199359 into knative:main May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.0] API for sequences.flows.knative.dev is v1
4 participants