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

Fix unable to deploy KafkaSink without Broker component #714

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Fix unable to deploy KafkaSink without Broker component #714

merged 1 commit into from
Mar 9, 2021

Conversation

pierDipi
Copy link
Member

@pierDipi pierDipi commented Mar 8, 2021

Fixes #712

Proposed Changes

  • Create a subdir for each component (Sink and Broker)
  • Create symlinks to common artifacts in each component subdir

Release Note

Fix unable to deploy KafkaSink without Kafka Broker installed

/kind bug

@pierDipi
Fix unable to deploy KafkaSink without Broker component
8497d43 
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@pierDipi pierDipi requested review from a team as code owners March 8, 2021 20:37
@knative-prow-robot knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. area/data-plane area/test size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2021
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 8, 2021
@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #714 (3340628) into main (c54a064) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #714   +/-   ##
=======================================
  Coverage   71.65%   71.65%           
=======================================
  Files          28       28           
  Lines        1242     1242           
=======================================
  Hits          890      890           
  Misses        274      274           
  Partials       78       78

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c54a064...8497d43. Read the comment docs.

@pierDipi
Copy link
Member Author

pierDipi commented Mar 8, 2021

Verified artifacts in https://console.cloud.google.com/storage/browser/knative-prow/pr-logs/pull/knative-sandbox_eventing-kafka-broker/714/pull-knative-sandbox-eventing-kafka-broker-integration-tests/1369024389797908480/artifacts/0af0050d-804e-11eb-bce1-8a835f25e258?pageState=(%22StorageObjectListTable%22:(%22f%22:%22%255B%255D%22))&prefix=&forceOnObjectsSortingFiltering=false

@pierDipi pierDipi changed the title [WIP] Fix unable to deploy KafkaSink without Broker component Fix unable to deploy KafkaSink without Broker component Mar 8, 2021
@knative-prow-robot 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 Mar 8, 2021
@pierDipi
Copy link
Member Author

pierDipi commented Mar 8, 2021

/cherry-pick release-0.21

@knative-prow-robot
Copy link
Contributor

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-0.21 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.21

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.

@pierDipi pierDipi mentioned this pull request Mar 8, 2021
Closed
slinkydeveloper
slinkydeveloper approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

/lgtm

test/e2e-common.sh
@@ -155,3 +155,11 @@ function export_logs_continuously() {
kubectl logs -n ${SYSTEM_NAMESPACE} -f -l=app="$deployment" >"$ARTIFACTS/${SYSTEM_NAMESPACE}/$deployment" 2>&1 &
done
}

function save_release_artifacts() {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's cool!

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slinkydeveloper

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021
@pierDipi
Copy link
Member Author

pierDipi commented Mar 9, 2021

/retest

3 similar comments
@pierDipi
Copy link
Member Author

pierDipi commented Mar 9, 2021

/retest

@pierDipi
Copy link
Member Author

pierDipi commented Mar 9, 2021

/retest

@pierDipi
Copy link
Member Author

pierDipi commented Mar 9, 2021

/retest

@knative-prow-robot knative-prow-robot merged commit 5919272 into knative-extensions:main Mar 9, 2021
@knative-prow-robot knative-prow-robot mentioned this pull request Mar 9, 2021
Merged
@knative-prow-robot
Copy link
Contributor

@pierDipi: new pull request created: #716

In response to this:

/cherry-pick release-0.21

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.

@lionelvillard
Copy link
Contributor

This installation works but not the sink. Getting:

E0315 14:38:33.635551       1 reflector.go:127] k8s.io/client-go@v0.19.7/tools/cache/reflector.go:156: Failed to watch *v1.Broker: failed to list *v1.Broker: the server could not find the requested resource (get brokers.eventing.knative.dev)
E0315 14:38:33.637178       1 reflector.go:127] k8s.io/client-go@v0.19.7/tools/cache/reflector.go:156: Failed to watch *v1.Trigger: failed to list *v1.Trigger: the server could not find the requested resource (get triggers.eventing.knative.dev)
E0315 14:38:34.970312       1 reflector.go:127] k8s.io/client-go@v0.19.7/tools/cache/reflector.go:156: Failed to watch *v1.Trigger: failed to list *v1.Trigger: the server could not find the requested resource (get triggers.eventing.knative.dev)

Disclaimer: I didn't try this PR. I just manually added the missing artifacts

@pierDipi
Copy link
Member Author

@lionelvillard that should be fine and it shouldn't affect the KafkaSink installation. Does it?
It can be removed by installing Broker and Trigger CRDs as we discussed in #286.

With that being said, I still think we should better handle this installation.

What about hosting Broker and Trigger CRD in our artifacts so that there will be no errors?

@pierDipi pierDipi deleted the KNATIVE-712 branch March 15, 2021 18:20
@lionelvillard
Copy link
Contributor

What about hosting Broker and Trigger CRD in our artifacts so that there will be no errors?

Maybe as part of the hypothetical standalone KafkaSink artifact?

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

@slinkydeveloper slinkydeveloper slinkydeveloper approved these changes

@fallback-notification-blocker fallback-notification-blocker Awaiting requested review from fallback-notification-blocker

Assignees

@slinkydeveloper slinkydeveloper

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/data-plane area/test cla: yes Indicates the PR's author has signed the CLA. kind/bug Categorizes issue or PR as related to a bug. 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.

Unable to deploy Kafka Sink data plane with v0.21.0
4 participants
@pierDipi @knative-prow-robot @lionelvillard @slinkydeveloper