Skip to content

chore: Add version information of eventing#495

Merged
knative-prow-robot merged 2 commits intoknative:masterfrom
rhuss:pr/eventing-version
Nov 19, 2019
Merged

chore: Add version information of eventing#495
knative-prow-robot merged 2 commits intoknative:masterfrom
rhuss:pr/eventing-version

Conversation

@rhuss
Copy link
Copy Markdown
Contributor

@rhuss rhuss commented Nov 12, 2019

Adding sources.eventing.knative.dev to version info (the only eventing API)
for now as this will be the first one used.

@rhuss
chore: Add version information of eventing
01f369d 
Adding sources.eventing.knative.dev to version info (the only eventing API)
for now as this will be the first one used.
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 12, 2019
@knative-prow-robot knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 12, 2019
@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Nov 12, 2019

/retest

3 similar comments
@rhuss
Copy link
Copy Markdown
Contributor Author

rhuss commented Nov 12, 2019

/retest

@rhuss
Copy link
Copy Markdown
Contributor Author

rhuss commented Nov 12, 2019

/retest

@rhuss
Copy link
Copy Markdown
Contributor Author

rhuss commented Nov 12, 2019

/retest

@sixolet
Copy link
Copy Markdown
Contributor

sixolet commented Nov 12, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2019
@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Nov 12, 2019

/retest

maximilien
maximilien approved these changes Nov 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

/lgtm

@maximilien
Copy link
Copy Markdown
Contributor

maximilien commented Nov 12, 2019

Failure in traffic split test. Hopefully not a transient issue and can be repro. Let me /retest

--- FAIL: TestTrafficSplit (352.08s)
    --- PASS: TestTrafficSplit/tag_two_revisions_as_v1_and_v2_and_give_50-50%_share (55.53s)
    --- PASS: TestTrafficSplit/ramp/up_down_a_revision_to_20%_adjusting_other_traffic_to_accommodate (13.26s)
    --- FAIL: TestTrafficSplit/tag_a_revision_as_candidate,_without_otherwise_changing_any_traffic_split (6.66s)
        traffic_split_test.go:368: assertion failed: 
            --- expectedTargets
            +++ formattedActualTargets
              []e2e.TargetFields{
              	{
              		Tag:      "",
            - 		Revision: "echo2-bpjfv-2",
            + 		Revision: "echo2-mfylg-1",
              		Percent:  100,
              		Latest:   true,
              	},
              	{Tag: "candidate", Revision: "echo2-mfylg-1"},
              }
            
    --- FAIL: TestTrafficSplit/tag_a_revision_as_candidate,_set_2%_traffic_adjusting_other_traffic_to_accommodate (6.97s)
        traffic_split_test.go:368: assertion failed: 
            --- expectedTargets
            +++ formattedActualTargets
              []e2e.TargetFields{
              	{
skipped 9 lines unfold_more
    --- PASS: TestTrafficSplit/update_tag_for_a_revision_from_candidate_to_current,_tag_current_is_present_on_another_revision (12.69s)
    --- PASS: TestTrafficSplit/update_tag_from_testing_to_staging_for_@latest_revision (9.49s)
    --- PASS: TestTrafficSplit/update_tag_from_testing_to_staging_for_a_revision_(non_@latest) (10.18s)
    --- PASS: TestTrafficSplit/remove_a_revision_with_tag_old_from_traffic_block_entirely (8.86s)
    --- PASS: TestTrafficSplit/tag_a_revision_as_stable_and_current_with_50-50%_traffic (7.45s)
    --- FAIL: TestTrafficSplit/revert_all_traffic_to_latest_ready_revision_of_service (6.16s)
        traffic_split_test.go:368: assertion failed: 
            --- expectedTargets
            +++ formattedActualTargets
              []e2e.TargetFields{
              	{
skipped 440 lines unfold_more

@maximilien
Copy link
Copy Markdown
Contributor

maximilien commented Nov 12, 2019

/test pull-knative-client-integration-tests

daisy-ycguo
daisy-ycguo reviewed Nov 13, 2019
View reviewed changes
Copy link
Copy Markdown

@daisy-ycguo daisy-ycguo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just wonder why not using eventing.knative.dev/v1alpha1 (knative-eventing v0.10.0) directly ?

There are three APIs in Eventing. Do you want to add three of them to API list when we add features of brokers and channels ?

  • sources.eventing.knative.dev/v1alpha1,
  • messaging.knative.dev/v1alpha1 and
  • eventing.knative.dev/v1alpha1.

I prefer to use eventing.knative.dev/v1alpha1 directly because I don't think end users want to know details. End users may hear about serving and eventing only. What's more, I believe sources, messaging and eventing should always be used the same version.

@daisy-ycguo daisy-ycguo mentioned this pull request Nov 13, 2019
Closed
@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Nov 15, 2019

/retest

1 similar comment
@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Nov 18, 2019

/retest

@daisy-ycguo
Copy link
Copy Markdown

daisy-ycguo commented Nov 18, 2019

/test pull-knative-client-integration-tests-latest-release

@rhuss
Copy link
Copy Markdown
Contributor Author

rhuss commented Nov 19, 2019

There are three APIs in Eventing. Do you want to add three of them to API list when we add features of brokers and channels?

Yes, indeed. The reason for focussing on API version (serving/v1beta1) instead of Release versions (Knative serving 0.10.0) is that the API version is the version the client and server need to agree. There can be multiple release versions supporting the same API version so its perfectly fine that a kn client compiled against knative-serving 0.9.0 can talk with a server side knative-serving 0.11.0 if they are using the same API version. Also, it is not possible to detect the server side release version.

The only reason why the release version is added to v1alpha1 versions is, that for v1alpha1 version the format can change for each release version (e.g. a v1alpha1 of knative-serving 0.7.0 is different from v1alpha1 of knative-serving 0.10.0). So for alpha version the release version is important.

For v1beta1 and v1 style version its expected that they do not change anymore. Instead if there is a change the API version is increased to e.g. v1beta2. This is theory as described in the Kubernetes docs but I'm not sure if Knative really freezes beta and GA versions as soon as they are released first (but let's assume that for now)

So for the user, the API version is the important version, i.e. those running on the cluster. Not the dependency release version against which a client is compiled.

But I agree, that we should group the API version accroding to serving and eventing, e.g. with a sublevel header. I will change the PR accordingly.

What's still missing is the the negotiation with the server-side about the version to use. At the moment, the client only uses a fixed version (e.g. v1alpha1 for serving) and hopes that the server supports this version (a server typically supports many versions, like 0.10.0 supports v1alpha1, v1beta1 and v1). We should query the CRD and check the supported server side versions here and also print those version to the user with kn version.

@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 19, 2019
@rhuss
Copy link
Copy Markdown
Contributor Author

rhuss commented Nov 19, 2019

@navidshaikh @daisy-ycguo you might want to have a look again. I added an extra grouping for serving & eventing to make it clearer which API version belongs to which Knative component.

@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Nov 19, 2019

Nice!

Version:      v20191119-local-9258086
Build Date:   2019-11-19 10:18:41
Git Revision: 9258086
Supported APIs:
* Serving
  - serving.knative.dev/v1alpha1 (knative-serving v0.10.0)
* Eventing
  - sources.eventing.knative.dev/v1alpha1 (knative-eventing v0.10.0)

navidshaikh
navidshaikh approved these changes Nov 19, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@navidshaikh navidshaikh 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 Nov 19, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Nov 19, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh, rhuss

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 [maximilien,navidshaikh,rhuss]

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 merged commit 4879726 into knative:master Nov 19, 2019
@navidshaikh
Copy link
Copy Markdown
Contributor

navidshaikh commented Nov 19, 2019

btw - The CI didnt flake for the latest run of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sixolet sixolet Awaiting requested review from sixolet

3 more reviewers

@daisy-ycguo daisy-ycguo daisy-ycguo left review comments

@maximilien maximilien maximilien approved these changes

@navidshaikh navidshaikh navidshaikh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@maximilien maximilien

@sixolet sixolet

@navidshaikh navidshaikh

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@rhuss @navidshaikh @sixolet @maximilien @daisy-ycguo @knative-prow-robot @googlebot