Add possibility to create services in release mode by vagababov · Pull Request #3070 · knative/serving

vagababov · 2019-02-02T00:20:09Z

For #2819
Implement @latestRevision shorthand to be able to create releases directly in the release mode.

I need integration tests, which will be either in this pr or separately.
Doc/spec update will be in separate pr.

/lint
-->

Fixes #

Proposed Changes

add a named target @latestRevision
when encountered it's routed to configuration, effectively picking the latest revision.

For example:

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: s-2
  namespace: default
spec:
  release:
    revisions:
    - "@latestRevision"
    configuration:
      revisionTemplate:
        spec:
          container:
            image: gcr.io/dm-vagababov/helloworld:the-one-i-love
            env:
            - name: TARGET
              value: "→test string←"

/cc @dgerd @mattmoor

knative-prow-robot

@vagababov: 0 warnings.

Details

In response to this:

For #2819
Implement @latestRevision shorthand to be able to create releases directly in the release mode.

I need integration tests, which will be either in this pr or separately.
Doc/spec update will be in separate pr.

/lint
-->

Fixes #

Proposed Changes

add a named target @latestRevision

when encountered it's routed to configuration, effectively picking the latest revision.

For example:
apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
 name: s-2
 namespace: default
spec:
 release:
   revisions:
   - "@latestRevision"
   configuration:
     revisionTemplate:
       spec:
         container:
           image: gcr.io/dm-vagababov/helloworld:the-one-i-love
           env:
           - name: TARGET
             value: "→test string←"
/cc @dgerd @mattmoor

Also /cc @sixolet @cooperneil

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vagababov · 2019-02-02T00:21:12Z

Also:
/cc @sixolet @cooperneil

knative-metrics-robot · 2019-02-02T00:25:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/service/resources/route.go	100.0%	92.6%	-7.4

vagababov · 2019-02-02T00:55:22Z

/test pull-knative-serving-go-coverage

dgerd

Looks good so far. Excited to see this get merged.

Don't forget to update https://github.com/knative/serving/blob/master/docs/spec/spec.md#service with the new documentation.

Co-Authored-By: vagababov <vagababov@users.noreply.github.com>

duglin · 2019-02-03T19:42:09Z

@vagababov to be clear... after this PR is merged

if someone was to update the revisionTemplate in the sample you gave it would roll out a new revision of the Service and all existing Pod would be replace with new ones
if someone was to update the "build" definition in that config, it would build a new image and then roll that out to replace all existing Pods

right?

vagababov · 2019-02-03T19:52:15Z

Yes, the example I have above would basically behave as runLatest (which kind of makes it obsolete, as happened with the pinned).

duglin · 2019-02-03T22:51:31Z

Great - thanks

markusthoemmes

I have the same comment regarding the releaseLatestRevisionKeyword constant. Seems to me as if it should only be defined once in one place vs. twice?

I need integration tests, which will be either in this pr or separately.
Doc/spec update will be in separate pr.

Why make them seperate PRs? I don't see an integration test being super urgent here given the quite good unit-test coverage but the doc change should land with this PR imo.

vagababov · 2019-02-04T17:52:07Z

I am totally fine with one place for constant, where should it be?
I'll update the spec as well (In general I prefer to separate code and documentation, though).

vagababov · 2019-02-04T18:03:09Z

spec.md updated. There wasn't a big section for release anyway.

vagababov · 2019-02-04T18:36:37Z

Added the comment.

Need to get back to the old istio

vagababov · 2019-02-04T19:58:23Z

PTAL.

dgerd · 2019-02-04T21:46:22Z

/lgtm

mattmoor

I'd like to verify the data plane is properly configured with @latest, but otherwise this LGTM for now.

I'd also like to see more complex traffic splitting cases with @latest, but I'm ok with that in follow-up.

mattmoor · 2019-02-04T22:35:35Z

 	)

-	objects, err := test.CreateRunLatestServiceReady(logger, clients, &names, &test.Options{})
+	objects, err := test.CreateReleaseServiceWithLatest(logger, clients, &names, &test.Options{})


Can we check that this serves the expected text?

I've added two checks. We can expand them in future, should we want to validate more fields.

vagababov · 2019-02-04T23:19:36Z

/retest

mattmoor

/lgtm
/approve

knative-prow-robot · 2019-02-05T05:52:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@latest

* base addition to the codebase with uts * add unit test to the validation * remove debug logging * raise test coverage * Update pkg/apis/serving/v1alpha1/service_validation.go Co-Authored-By: vagababov <vagababov@users.noreply.github.com> * fix the const name * update spec.md with the new constant reference * address comments. create a globally known constant * fix the test name * go with @latest for brevity * update the conformance test * add conformance test, that validates release service creation * added more validation to the service shape

vagababov added 3 commits February 1, 2019 10:43

base addition to the codebase with uts

6e0d07d

add unit test to the validation

e1014c3

Merge branch 'master' into 2819-add-latest

00e6eb5

knative-prow-robot requested review from dgerd and mattmoor February 2, 2019 00:20

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2019

knative-prow-robot reviewed Feb 2, 2019

View reviewed changes

knative-prow-robot requested review from cooperneil and sixolet February 2, 2019 00:21

remove debug logging

2185590

raise test coverage

fede166

dgerd reviewed Feb 2, 2019

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/service_validation.go Outdated

Dan Gerdesmeier and others added 2 commits February 1, 2019 22:14

Update pkg/apis/serving/v1alpha1/service_validation.go

c3894b6

Co-Authored-By: vagababov <vagababov@users.noreply.github.com>

fix the const name

1ed0d74

markusthoemmes reviewed Feb 4, 2019

View reviewed changes

update spec.md with the new constant reference

1465cad

mattmoor reviewed Feb 4, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/resources/route.go Outdated

mattmoor reviewed Feb 4, 2019

View reviewed changes

Comment thread pkg/apis/serving/v1alpha1/service_validation_test.go Outdated

vagababov added 2 commits February 4, 2019 10:35

address comments. create a globally known constant

7d84da7

fix the test name

4605cff

mattmoor reviewed Feb 4, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/resources/route.go

vagababov changed the title ~~WIP: add possibility to create services in release mode~~ Add possibility to create services in release mode Feb 4, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2019

mattmoor reviewed Feb 4, 2019

View reviewed changes

Comment thread docs/spec/spec.md Outdated

go with @latest for brevity

6b26939

mattmoor reviewed Feb 4, 2019

View reviewed changes

Comment thread docs/spec/spec.md

vagababov added 3 commits February 4, 2019 11:29

update the conformance test

f4ff69c

Merge branch 'master' into 2819-add-latest

98d8d62

Need to get back to the old istio

add conformance test, that validates release service creation

5b85607

knative-prow-robot assigned dgerd Feb 4, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2019

mattmoor reviewed Feb 4, 2019

View reviewed changes

added more validation to the service shape

1503544

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2019

knative-prow-robot assigned mattmoor Feb 5, 2019

mattmoor reviewed Feb 5, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2019

knative-prow-robot merged commit 5e9d580 into knative:master Feb 5, 2019

vagababov deleted the 2819-add-latest branch February 13, 2019 17:53

dgerd mentioned this pull request Feb 14, 2019

Add dgerd to list of serving-api-approvers #3228

Merged

Conversation

vagababov commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

vagababov commented Feb 2, 2019

Uh oh!

knative-metrics-robot commented Feb 2, 2019

Uh oh!

vagababov commented Feb 2, 2019

Uh oh!

dgerd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

duglin commented Feb 3, 2019

Uh oh!

vagababov commented Feb 3, 2019

Uh oh!

duglin commented Feb 3, 2019

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

vagababov commented Feb 4, 2019

Uh oh!

vagababov commented Feb 4, 2019

Uh oh!

Uh oh!

Uh oh!

vagababov commented Feb 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vagababov commented Feb 4, 2019

Uh oh!

dgerd commented Feb 4, 2019

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattmoor Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

vagababov Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vagababov commented Feb 4, 2019

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Feb 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vagababov commented Feb 2, 2019 •

edited

Loading