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.

Sign up for GitHub

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

Jump to bottom

Break some more knative/build dependencies #636

Merged
merged 4 commits into from
Mar 20, 2019
Merged

Break some more knative/build dependencies #636

merged 4 commits into from
Mar 20, 2019

Conversation

vdemeester
Copy link
Member

Changes

Part of #252 but not complete yet — what this does :

  • Remove GetBuildSpec from task_types, and thus remove knative/buildimport frompkg/apis` package
  • Remove using creating a BuildStatus from Pod to then convert that to a TaskRunStatus, let's be more direct.

cc @imjasonh

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.

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2019
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 19, 2019
@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 19, 2019
@nader-ziada
Copy link
Member

Are you planning on making the other changes in the issue in other PRs?

@vdemeester
Copy link
Member Author

@pivotal-nader-ziada if this one looks good and is green, I'll open a follow-up PR (as it can be done separately). I just wanted to have a "not-too-big" PR 👼

@shashwathi
Copy link
Contributor

These changes are good to go by itself too so follow up PR for further changes SGTM.

bobcatfish
bobcatfish reviewed Mar 19, 2019
View reviewed changes
Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

Just a few minor comments, looks great! :D

One thought, do you think (if we had coverage mesasurements haha) that the coverage would go up or down with this PR? (or be the same cuz we were using build logic before anyway that maybe wasnt fully covered?) It seems like maybe there is some potential for more unit test coverage

pkg/reconciler/v1alpha1/taskrun/taskrun.go
taskRun.Status.SetCondition(&duckv1alpha1.Condition{
Type: duckv1alpha1.ConditionSucceeded,
Status: corev1.ConditionUnknown,
Reason: "Building",
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe Running instead of Building?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't change what we had before when using BuildStatus but yeah I agree.

@vdemeester
Copy link
Member Author

One thought, do you think (if we had coverage mesasurements haha) that the coverage would go up or down with this PR? (or be the same cuz we were using build logic before anyway that maybe wasnt fully covered?) It seems like maybe there is some potential for more unit test coverage

That's a very good point 👼 I'll do some manual measure to make sure i don't lower the coverage.

@vdemeester
Remove GetBuildSpec from task_types… ✂️
bf124e6 
… and thus remove `knative/build` import from `pkg/apis` package

Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the 252-break-knative-build-dep branch from 17d45df to a10b83d Compare March 20, 2019 11:16
@vdemeester
Remove using BuildStatus for TaskRunStatus ✂️
bf379c1 
Using directly Pod to build TaskRunStatus

Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the 252-break-knative-build-dep branch from a10b83d to ab60241 Compare March 20, 2019 11:19
@vdemeester
Copy link
Member Author

For coverage (only changed packages):

Package Before After status
pkgs/apis/pipeline/v1alpha1 32.5 32.8 😄
pkgs/reconciler/v1alpha1/taskrun 81.1 74.2 😓
pkgs/reconciler/v1alpha1/resources 89.0 87.9 😅

@nader-ziada
Copy link
Member

@vdemeester are you still going to add tests?

@vdemeester
Copy link
Member Author

@pivotal-nader-ziada I'm thinking about it 😅 I'm just running out of time today 😅

@vdemeester
Update codegen 😅
376d172 
Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the 252-break-knative-build-dep branch from ab60241 to 376d172 Compare March 20, 2019 15:56
@vdemeester
Copy link
Member Author

vdemeester commented Mar 20, 2019 •
edited
Loading

Added some more test, coverage update

Package Before After status
pkgs/apis/pipeline/v1alpha1 32.5 32.8 😄
pkgs/reconciler/v1alpha1/taskrun 81.1 82.7 😄
pkgs/reconciler/v1alpha1/resources 89.0 90.2 😄

@vdemeester
Add more coverage on updateStatusFromPod 💃
49c71cf 
Also, removed a dead code :)

Signed-off-by: Vincent Demeester <[email protected]>
@vdemeester vdemeester force-pushed the 252-break-knative-build-dep branch from 4a1a702 to 49c71cf Compare March 20, 2019 16:40
@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 20, 2019
@nader-ziada
Copy link
Member

/lgtm

Looking forward to the follow up PR :)

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2019
@tekton-robot tekton-robot merged commit 0fd64c0 into tektoncd:master Mar 20, 2019
@vdemeester vdemeester deleted the 252-break-knative-build-dep branch March 20, 2019 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobcatfish bobcatfish bobcatfish left review comments

@imjasonh imjasonh imjasonh requested changes

@dlorenc dlorenc Awaiting requested review from dlorenc

@nader-ziada nader-ziada Awaiting requested review from nader-ziada

Assignees

@nader-ziada nader-ziada

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vdemeester @tekton-robot @nader-ziada @shashwathi @imjasonh @bobcatfish @googlebot