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

Move Condition stuff to apis, add a v1beta1 Status. #361

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

mattmoor
Copy link
Member

@mattmoor mattmoor commented Apr 2, 2019

This moves the common Condition stuff to apis, and creates a v1beta1 form of Status that uses the Condition it defines (changing this in v1alpha1 is too breaking).

There aren't really any meaningful changes in this PR, mostly reorganization. Enumerating what I did:

  1. Copied condition_set*.go to apis/,
  2. Copied the Condition portions of conditions_types.go to apis/,
  3. Copied the balance of conditions_types.go to apis/duck/v1beta1/status_types.go,
  4. Changed the parts of the above to reference things in the appropriate new places,
  5. Removed the reflection-based ConditionsAccessor stuff, implementing it instead on duckv1beta1.Status.
  6. Incorporate: Stop initializing conditions imperatively. #358

cc @vdemeester (since you commented on the predecessors)

FYI I was able to import this into serving without any code changes, then lots to take advantage of it.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 2, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 2, 2019
@mattmoor
Copy link
Member Author

mattmoor commented Apr 2, 2019

/hold

Putting a hold on this, but I'd like to land it once we cut 0.5

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2019
@mattmoor
Copy link
Member Author

mattmoor commented Apr 2, 2019

You can see my WIP import into serving here, although I'll probably split it across a few PRs.

@mattmoor
Copy link
Member Author

mattmoor commented Apr 2, 2019

I'll fix coverage too :(

@mattmoor mattmoor force-pushed the cond-v1beta1 branch 3 times, most recently from 588cba1 to 7947aaf Compare April 2, 2019 00:47
@mattmoor
Copy link
Member Author

mattmoor commented Apr 2, 2019

This needs: #358

UPDATE: pushing the above out, we're not ready for it in serving. :(

This moves the common Condition stuff to apis, and creates a v1beta1 form of Status that uses the Condition it defines (changing this in v1alpha1 is too breaking).

There aren't really any meaningful changes in this PR, mostly reorganization.  Enumerating what I did:
1. Copied `condition_set*.go` to `apis/`,
1. Copied the `Condition` portions of `conditions_types.go` to `apis/`,
1. Copied the balance of `conditions_types.go` to `apis/duck/v1beta1/status_types.go`,
1. Changed the parts of the above to reference things in the appropriate new places,
1. Removed the reflection-based `ConditionsAccessor` stuff, implementing it instead on `duckv1beta1.Status`.
1. Incorporate: knative#358
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
apis/condition_set.go Do not exist 100.0%
apis/condition_types.go Do not exist 100.0%
apis/duck/v1alpha1/register.go 0.0% 100.0% 100.0
apis/duck/v1beta1/register.go Do not exist 100.0%
apis/duck/v1beta1/status_types.go Do not exist 100.0%

@mattmoor
Copy link
Member Author

mattmoor commented Apr 2, 2019

/hold cancel

This creates a second parallel implementation, so it won't disrupt anyone if it goes in early.

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2019
mattmoor added a commit to mattmoor/build that referenced this pull request Apr 2, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361
mattmoor added a commit to mattmoor/build that referenced this pull request Apr 2, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361
@n3wscott
Copy link
Contributor

n3wscott commented Apr 2, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2019
@knative-prow-robot knative-prow-robot merged commit 281cda8 into knative:master Apr 2, 2019
@mattmoor mattmoor deleted the cond-v1beta1 branch April 2, 2019 17:30
mattmoor added a commit to mattmoor/build that referenced this pull request Apr 2, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361
mattmoor added a commit to mattmoor/build that referenced this pull request Apr 4, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361
mattmoor added a commit to mattmoor/build that referenced this pull request Apr 4, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361
vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Apr 9, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Apr 10, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Apr 10, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
vdemeester added a commit to vdemeester/tektoncd-pipeline that referenced this pull request Apr 16, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
tekton-robot pushed a commit to tektoncd/pipeline that referenced this pull request Apr 17, 2019
See here for more context on what's changed (no breaking changes): knative/pkg#361

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
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. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants