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.

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

Adding a new Minikube flag for helm chart install of Apache Camel K #521

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

matzew
Copy link
Contributor

@matzew matzew commented Jun 10, 2024

Changes

  • 🎁 Add new feature for Apache Camel K install, for allowing advanced Knative Eventing samples

/kind

Fixes #

Release Note


Docs


Copy link

knative-prow bot commented Jun 10, 2024

@matzew: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them.

In response to this:

Changes

  • 🎁 Add new feature for Apache Camel K install, for allowing advanced Knative Eventing samples

/kind

Fixes #

Release Note


Docs


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-sigs/prow repository.

@knative-prow knative-prow bot requested review from dsimansk and rhuss June 10, 2024 08:26
@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 10, 2024
@matzew matzew force-pushed the add_camel_k_helmchart branch from db76297 to f61714b Compare June 10, 2024 08:31
pkg/install/install.go Outdated Show resolved Hide resolved
@matzew matzew force-pushed the add_camel_k_helmchart branch from f61714b to b3723a7 Compare June 10, 2024 10:06
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2024
README.md Show resolved Hide resolved
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2024
@matzew matzew force-pushed the add_camel_k_helmchart branch from b3723a7 to fa35625 Compare June 19, 2024 13:52
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2024
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2024
@matzew matzew changed the title Adding a new flag for helm chart install of Apache Camel K Adding a new Minikube flag for helm chart install of Apache Camel K Jun 19, 2024
@matzew
Copy link
Contributor Author

matzew commented Jun 19, 2024

How to test this PR?

  • Build binary (./hack/build.sh)
  • make sure helm is installed
  • run the add-on: ./kn-quickstart minikube --install-eventing --install-camel

Once all installed, run a simple example, such as kamelet as source.

Convenience manifest for all bits:

apiVersion: eventing.knative.dev/v1
kind: Broker
metadata:
  namespace: default
  name: kamelet-source
---
apiVersion: v1
kind: Service
metadata:
  name: kamelet-receiver-function
spec:
  selector:
    app: kamelet-receiver-function
  ports:
    - port: 80
      protocol: TCP
      targetPort: http
      name: http
---
apiVersion: v1
kind: Pod
metadata:
  name: kamelet-receiver-function
  labels:
    app: kamelet-receiver-function
spec:
  securityContext:
    runAsNonRoot: true
    seccompProfile:
      type: RuntimeDefault
  containers:
  - name: kamelet-receiver-function
    securityContext:
      allowPrivilegeEscalation: false
      readOnlyRootFilesystem: true
      runAsNonRoot: true
      capabilities:
        drop:
        - ALL
      seccompProfile:
        type: RuntimeDefault
    image: registry.ci.openshift.org/openshift/knative-eventing-event-display:knative-nightly
    imagePullPolicy: Always
    ports:
    - containerPort: 8080
      protocol: TCP
      name: http
---
apiVersion: eventing.knative.dev/v1
kind: Trigger
metadata:
  name: trigger
spec:
  broker: kamelet-source
  subscriber:
    ref:
      apiVersion: v1
      kind: Service
      name: kamelet-receiver-function
---
apiVersion: camel.apache.org/v1
kind: Pipe
metadata:
  name: beer-source-pipe
spec:
  source:
    ref:
      kind: Kamelet
      apiVersion: camel.apache.org/v1
      name: beer-source
  sink:
    properties:
      cloudEventsType: com.corp.my.beer.source
    ref:
      kind: Broker
      apiVersion: eventing.knative.dev/v1
      name: kamelet-source

Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

Following the instructions above works on my install of minikube - all the resources become ready and I see events showing up in the receiver function

@Leo6Leo
Copy link

Leo6Leo commented Jun 19, 2024

{"level":"error","ts":"2024-06-19T15:31:45Z","msg":"Reconciler error","controller":"kamelet-binding-controller","object":{"name":"beer-source-pipe","namespace":"default"},"namespace":"default","name":"beer-source-pipe","reconcileID":"a8133d24-2ce3-4784-aae1-571a44299a09","error":"could not determine sink URI: integration referencing Knative endpoint 'kamelet-source' that cannot run, because Knative is not installed on the cluster","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:324\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:265\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:226"}

After applying the pipe yaml, camel-operator is complaining "Knative is not installed".

So only one thing: the testing command should add --install-serving flag: ./kn-quickstart minikube --install-serving --install-eventing --install-camel!

Other than that, all the resources become ready and I can see events showing up in the receiver function log too! @matzew

@matzew
Copy link
Contributor Author

matzew commented Jun 19, 2024 via email

@Leo6Leo
Copy link

Leo6Leo commented Jun 19, 2024

nope. can you update your helm chart and try again? That serving issues was fixed by camel

-M Sent from Gmail Mobile
On Wed 19. Jun 2024 at 17:42, Leo Li @.> wrote: {"level":"error","ts":"2024-06-19T15:31:45Z","msg":"Reconciler error","controller":"kamelet-binding-controller","object":{"name":"beer-source-pipe","namespace":"default"},"namespace":"default","name":"beer-source-pipe","reconcileID":"a8133d24-2ce3-4784-aae1-571a44299a09","error":"could not determine sink URI: integration referencing Knative endpoint 'kamelet-source' that cannot run, because Knative is not installed on the @.@.@./pkg/internal/controller/controller.go:226 @.@.@./pkg/internal/controller/controller.go:226>"} After applying the pipe yaml, camel-operator is complaining "Knative is not installed". So only one thing: the testing command should add --install-serving flag: ./kn-quickstart minikube --install-serving --install-eventing --install-camel! Other than that, all the resources become ready and I can see events showing up in the receiver function log too! @matzew https://github.com/matzew — Reply to this email directly, view it on GitHub <#521 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGPTUS6DMVSULN2VEIYK3ZIGRE7AVCNFSM6AAAAABJBZ2X36VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZZGAYDMOBTHE . You are receiving this because you were mentioned.Message ID: @.>

It works out after upgrading the helm chart. Thanks! @matzew Now I can say that all the resources become ready and I can see events showing up in the receiver function log with minikube.

@@ -237,3 +265,30 @@ func waitForCRDsEstablished() error {
func waitForPodsReady(ns string) error {
return runCommand(exec.Command("kubectl", "wait", "pod", "--timeout=10m", "--for=condition=Ready", "-l", "!job-name", "-n", ns))
}

func runHelmInstall(registryAddress string) error {
cmd := exec.Command("helm", "install",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mark the exec command with linter ignore command just to avoid further golangci-linter errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition I'd not mind checking if helm is installed first and displaying "Please install helm CLI" for a nicer UX.

Not a blocking issue though.

Copy link
Contributor

@dsimansk dsimansk left a comment

Choose a reason for hiding this comment

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

Looks good, I've added a few comment to consider from UX point of view. But that can be polished over time.

@@ -55,6 +56,10 @@ func installEventingOption(targetCmd *cobra.Command) {
targetCmd.Flags().BoolVar(&installEventing, "install-eventing", false, "install Eventing on quickstart cluster")
}

func installCamelOption(targetCmd *cobra.Command) {
targetCmd.Flags().BoolVar(&installCamel, "install-camel", false, "install Apache Camel K on quickstart cluster")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer something like --extensions camel. That could potentially server for additional 3rd party installations. But not a hard opinion, we probably won't grow this list to unmanageable state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is a good idea for a future improvement. Sounds good?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll create a new issue from this suggestion. All good for the scope of this PR.

Signed-off-by: Matthias Wessendorf <[email protected]>
@dsimansk
Copy link
Contributor

/approve
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2024
Copy link

knative-prow bot commented Jun 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, matzew

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 knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2024
@knative-prow knative-prow bot merged commit 136c4ab into knative-extensions:main Jun 20, 2024
22 checks passed
matzew added a commit to matzew/kn-plugin-quickstart that referenced this pull request Nov 27, 2024
knative-prow bot pushed a commit that referenced this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants