Skip to content

Start to refactor the way builds are created. #2400

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

Merged
knative-prow-robot merged 1 commit into from
Nov 5, 2018
Merged

Start to refactor the way builds are created. #2400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/reconciler/v1alpha1/configuration/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,19 +218,26 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati
func (c *Reconciler) createRevision(ctx context.Context, config *v1alpha1.Configuration, revName string) (*v1alpha1.Revision, error) {
logger := logging.FromContext(ctx)

var buildRef *corev1.ObjectReference
if config.Spec.Build != nil {
// TODO(mattmoor): Determine whether we reuse the previous build.
build := resources.MakeBuild(config)
gvr, _ := meta.UnsafeGuessKindToResource(build.GroupVersionKind())

created, err := c.DynamicClientSet.Resource(gvr).Namespace(build.GetNamespace()).Create(build)
if err != nil {
return nil, err
}
logger.Infof("Created Build:\n%+v", created.GetName())
c.Recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Build %q", created.GetName())
buildRef = &corev1.ObjectReference{
APIVersion: created.GetAPIVersion(),
Kind: created.GetKind(),
Name: created.GetName(),
}
}

rev := resources.MakeRevision(config)
rev := resources.MakeRevision(config, buildRef)
created, err := c.ServingClientSet.ServingV1alpha1().Revisions(config.Namespace).Create(rev)
if err != nil {
return nil, err
Expand Down
52 changes: 28 additions & 24 deletions pkg/reconciler/v1alpha1/configuration/configuration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TestReconcile(t *testing.T) {
cfg("no-revisions-yet", "foo", 1234),
},
WantCreates: []metav1.Object{
resources.MakeRevision(cfg("no-revisions-yet", "foo", 1234)),
resources.MakeRevision(cfg("no-revisions-yet", "foo", 1234), nil),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("no-revisions-yet", "foo", 1234,
Expand All @@ -93,7 +93,7 @@ func TestReconcile(t *testing.T) {
setConcurrencyModel(cfg("validation-failure", "foo", 1234), "Bogus"),
},
WantCreates: []metav1.Object{
setRevConcurrencyModel(resources.MakeRevision(cfg("validation-failure", "foo", 1234)), "Bogus"),
setRevConcurrencyModel(resources.MakeRevision(cfg("validation-failure", "foo", 1234), nil), "Bogus"),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: setConcurrencyModel(cfgWithStatus("validation-failure", "foo", 1234,
Expand All @@ -114,7 +114,11 @@ func TestReconcile(t *testing.T) {
},
WantCreates: []metav1.Object{
resources.MakeBuild(cfgWithBuild("need-rev-and-build", "foo", 99998, &buildSpec)),
resources.MakeRevision(cfgWithBuild("need-rev-and-build", "foo", 99998, &buildSpec)),
resources.MakeRevision(cfgWithBuild("need-rev-and-build", "foo", 99998, &buildSpec), &corev1.ObjectReference{
APIVersion: "build.knative.dev/v1alpha1",
Kind: "Build",
Name: "need-rev-and-build-99998",
}),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithBuildAndStatus("need-rev-and-build", "foo", 99998, &buildSpec,
Expand All @@ -133,7 +137,7 @@ func TestReconcile(t *testing.T) {
Name: "reconcile revision matching generation (ready: unknown)",
Objects: []runtime.Object{
cfg("matching-revision-not-done", "foo", 5432),
setRevCreationTimestamp(resources.MakeRevision(cfg("matching-revision-not-done", "foo", 5432)), time.Now()),
setRevCreationTimestamp(resources.MakeRevision(cfg("matching-revision-not-done", "foo", 5432), nil), time.Now()),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("matching-revision-not-done", "foo", 5432,
Expand All @@ -152,7 +156,7 @@ func TestReconcile(t *testing.T) {
Name: "reconcile revision matching generation (ready: true)",
Objects: []runtime.Object{
cfg("matching-revision-done", "foo", 5555),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("matching-revision-done", "foo", 5555))), time.Now()),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("matching-revision-done", "foo", 5555), nil)), time.Now()),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("matching-revision-done", "foo", 5555,
Expand Down Expand Up @@ -182,14 +186,14 @@ func TestReconcile(t *testing.T) {
}},
},
),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("matching-revision-done-idempotent", "foo", 5566))), time.Now()),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("matching-revision-done-idempotent", "foo", 5566), nil)), time.Now()),
},
Key: "foo/matching-revision-done-idempotent",
}, {
Name: "reconcile revision matching generation (ready: false)",
Objects: []runtime.Object{
cfg("matching-revision-failed", "foo", 5555),
setRevCreationTimestamp(makeRevFailed(resources.MakeRevision(cfg("matching-revision-failed", "foo", 5555))), time.Now()),
setRevCreationTimestamp(makeRevFailed(resources.MakeRevision(cfg("matching-revision-failed", "foo", 5555), nil)), time.Now()),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("matching-revision-failed", "foo", 5555,
Expand All @@ -210,7 +214,7 @@ func TestReconcile(t *testing.T) {
Name: "reconcile revision matching generation (ready: bad)",
Objects: []runtime.Object{
cfg("bad-condition", "foo", 5555),
makeRevStatus(resources.MakeRevision(cfg("bad-condition", "foo", 5555)),
makeRevStatus(resources.MakeRevision(cfg("bad-condition", "foo", 5555), nil),
v1alpha1.RevisionStatus{
Conditions: duckv1alpha1.Conditions{{
Type: v1alpha1.RevisionConditionReady,
Expand Down Expand Up @@ -271,7 +275,7 @@ func TestReconcile(t *testing.T) {
cfg("create-revision-failure", "foo", 99998),
},
WantCreates: []metav1.Object{
resources.MakeRevision(cfg("create-revision-failure", "foo", 99998)),
resources.MakeRevision(cfg("create-revision-failure", "foo", 99998), nil),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("create-revision-failure", "foo", 99998,
Expand All @@ -297,7 +301,7 @@ func TestReconcile(t *testing.T) {
cfg("update-config-failure", "foo", 1234),
},
WantCreates: []metav1.Object{
resources.MakeRevision(cfg("update-config-failure", "foo", 1234)),
resources.MakeRevision(cfg("update-config-failure", "foo", 1234), nil),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("update-config-failure", "foo", 1234,
Expand Down Expand Up @@ -327,7 +331,7 @@ func TestReconcile(t *testing.T) {
}},
},
),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("revision-recovers", "foo", 1337))), time.Now()),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("revision-recovers", "foo", 1337), nil)), time.Now()),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("revision-recovers", "foo", 1337,
Expand Down Expand Up @@ -362,9 +366,9 @@ func TestGCReconcile(t *testing.T) {
Name: "dont gc old revision no pin",
Objects: []runtime.Object{
cfg("old-revision", "foo", 5556),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5554))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5555))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5554), nil)), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5555), nil)), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556), nil)), time.Now().Add(-10*time.Minute)),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("old-revision", "foo", 5556,
Expand All @@ -384,7 +388,7 @@ func TestGCReconcile(t *testing.T) {
Name: "dont gc old latest revision no pin",
Objects: []runtime.Object{
cfg("old-revision", "foo", 5556),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556), nil)), time.Now().Add(-10*time.Minute)),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("old-revision", "foo", 5556,
Expand All @@ -404,8 +408,8 @@ func TestGCReconcile(t *testing.T) {
Name: "dont gc revisions minimum generations no pin",
Objects: []runtime.Object{
cfg("old-revision", "foo", 5556),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5555))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5555), nil)), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556), nil)), time.Now().Add(-10*time.Minute)),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("old-revision", "foo", 5556,
Expand All @@ -426,10 +430,10 @@ func TestGCReconcile(t *testing.T) {
Objects: []runtime.Object{
cfg("old-revision", "foo", 5556),
setRevLastPinned(setRevCreationTimestamp(
makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5554))),
makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5554), nil)),
time.Now().Add(-10*time.Minute)), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5555))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556))), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5555), nil)), time.Now().Add(-10*time.Minute)),
setRevCreationTimestamp(makeRevReady(t, resources.MakeRevision(cfg("old-revision", "foo", 5556), nil)), time.Now().Add(-10*time.Minute)),
},
WantUpdates: []clientgotesting.UpdateActionImpl{{
Object: cfgWithStatus("old-revision", "foo", 5556,
Expand Down Expand Up @@ -612,7 +616,7 @@ func TestIsRevisionStale(t *testing.T) {
serving.ConfigurationGenerationLabelKey: "1",
},
Annotations: map[string]string{
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
},
},
},
Expand All @@ -627,7 +631,7 @@ func TestIsRevisionStale(t *testing.T) {
serving.ConfigurationGenerationLabelKey: "1",
},
Annotations: map[string]string{
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()),
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()),
},
},
},
Expand All @@ -642,7 +646,7 @@ func TestIsRevisionStale(t *testing.T) {
serving.ConfigurationGenerationLabelKey: "2",
},
Annotations: map[string]string{
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
},
},
},
Expand All @@ -657,7 +661,7 @@ func TestIsRevisionStale(t *testing.T) {
serving.ConfigurationGenerationLabelKey: "1",
},
Annotations: map[string]string{
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
"serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()),
},
},
},
Expand Down
20 changes: 2 additions & 18 deletions pkg/reconciler/v1alpha1/configuration/resources/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
corev1 "k8s.io/api/core/v1"
)

func MakeRevision(config *v1alpha1.Configuration) *v1alpha1.Revision {
func MakeRevision(config *v1alpha1.Configuration, buildRef *corev1.ObjectReference) *v1alpha1.Revision {
Copy link
Copy Markdown
Contributor

@knative-prow-robot knative-prow-robot Nov 4, 2018

Choose a reason for hiding this comment

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

Golint comments: exported function MakeRevision should have comment or be unexported. More info.

// Start from the ObjectMeta/Spec inlined in the Configuration resources.
rev := &v1alpha1.Revision{
ObjectMeta: config.Spec.RevisionTemplate.ObjectMeta,
Expand Down Expand Up @@ -54,24 +54,8 @@ func MakeRevision(config *v1alpha1.Configuration) *v1alpha1.Revision {
// Populate OwnerReferences so that deletes cascade.
rev.OwnerReferences = append(rev.OwnerReferences, *kmeta.NewControllerRef(config))

// Fill in the build name, if specified.
rev.Spec.BuildName = names.Build(config)

// Fill in buildRef if build is involved
rev.Spec.BuildRef = buildRef(config)
rev.Spec.BuildRef = buildRef

return rev
}

func buildRef(config *v1alpha1.Configuration) *corev1.ObjectReference {
if config.Spec.Build == nil {
return nil
}

b := MakeBuild(config)
return &corev1.ObjectReference{
APIVersion: b.GetAPIVersion(),
Kind: b.GetKind(),
Name: b.GetName(),
}
}
37 changes: 21 additions & 16 deletions pkg/reconciler/v1alpha1/configuration/resources/revision_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ func TestMakeRevisions(t *testing.T) {
tests := []struct {
name string
configuration *v1alpha1.Configuration
buildRef *corev1.ObjectReference
want *v1alpha1.Revision
}{{
name: "no build",
Expand All @@ -52,8 +53,8 @@ func TestMakeRevisions(t *testing.T) {
},
want: &v1alpha1.Revision{
ObjectMeta: metav1.ObjectMeta{
Namespace: "no",
Name: "build-00012",
Namespace: "no",
Name: "build-00012",
Annotations: map[string]string{},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: v1alpha1.SchemeGroupVersion.String(),
Expand All @@ -63,9 +64,9 @@ func TestMakeRevisions(t *testing.T) {
BlockOwnerDeletion: &boolTrue,
}},
Labels: map[string]string{
serving.ConfigurationLabelKey: "build",
serving.ConfigurationLabelKey: "build",
serving.ConfigurationGenerationLabelKey: "12",
serving.ServiceLabelKey: "",
serving.ServiceLabelKey: "",
},
},
Spec: v1alpha1.RevisionSpec{
Expand Down Expand Up @@ -97,10 +98,15 @@ func TestMakeRevisions(t *testing.T) {
},
},
},
buildRef: &corev1.ObjectReference{
APIVersion: "build.knative.dev/v1alpha1",
Kind: "Build",
Name: "build-00099",
},
want: &v1alpha1.Revision{
ObjectMeta: metav1.ObjectMeta{
Namespace: "with",
Name: "build-00099",
Namespace: "with",
Name: "build-00099",
Annotations: map[string]string{},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: v1alpha1.SchemeGroupVersion.String(),
Expand All @@ -110,13 +116,12 @@ func TestMakeRevisions(t *testing.T) {
BlockOwnerDeletion: &boolTrue,
}},
Labels: map[string]string{
serving.ConfigurationLabelKey: "build",
serving.ConfigurationLabelKey: "build",
serving.ConfigurationGenerationLabelKey: "99",
serving.ServiceLabelKey: "",
serving.ServiceLabelKey: "",
},
},
Spec: v1alpha1.RevisionSpec{
BuildName: "build-00099",
BuildRef: &corev1.ObjectReference{
APIVersion: "build.knative.dev/v1alpha1",
Kind: "Build",
Expand Down Expand Up @@ -153,8 +158,8 @@ func TestMakeRevisions(t *testing.T) {
},
want: &v1alpha1.Revision{
ObjectMeta: metav1.ObjectMeta{
Namespace: "with",
Name: "labels-00099",
Namespace: "with",
Name: "labels-00099",
Annotations: map[string]string{},
OwnerReferences: []metav1.OwnerReference{{
APIVersion: v1alpha1.SchemeGroupVersion.String(),
Expand All @@ -164,9 +169,9 @@ func TestMakeRevisions(t *testing.T) {
BlockOwnerDeletion: &boolTrue,
}},
Labels: map[string]string{
serving.ConfigurationLabelKey: "labels",
serving.ConfigurationLabelKey: "labels",
serving.ConfigurationGenerationLabelKey: "99",
serving.ServiceLabelKey: "",
serving.ServiceLabelKey: "",

"foo": "bar",
"baz": "blah",
Expand Down Expand Up @@ -214,9 +219,9 @@ func TestMakeRevisions(t *testing.T) {
BlockOwnerDeletion: &boolTrue,
}},
Labels: map[string]string{
serving.ConfigurationLabelKey: "annotations",
serving.ConfigurationLabelKey: "annotations",
serving.ConfigurationGenerationLabelKey: "99",
serving.ServiceLabelKey: "",
serving.ServiceLabelKey: "",
},
Annotations: map[string]string{
"foo": "bar",
Expand All @@ -233,7 +238,7 @@ func TestMakeRevisions(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := MakeRevision(test.configuration)
got := MakeRevision(test.configuration, test.buildRef)
if diff := cmp.Diff(test.want, got); diff != "" {
t.Errorf("MakeRevision (-want, +got) = %v", diff)
}
Expand Down