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

hack: development using "make run" (with loki) #61

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

eranra
Copy link
Contributor

@eranra eranra commented Feb 17, 2022

  1. Updating the Makefile to allow deploy and undeploy of sample cr (there was some unneeded | etch command)
  2. Adding Development documentation inside hack folder
  3. Adding simple Loki deployment
  4. Adding simple Loki Grafana deployment script

@jotak
Copy link
Member

jotak commented Feb 21, 2022

Currently we have some hacks in https://github.com/netobserv/documents , especially for loki & grafana, so I don't think we want to have that in two different places, however maybe it makes more sense to have it in the operator repo indeed.

Beside that, we haven't introduced much shell scripts yet, maybe we can keep using the makefile to add loki/grafana deploying targets? We could copy here the zero-click loki/grafana templates (https://github.com/netobserv/documents/tree/main/examples/zero-click-loki) and simply execute kubectl on that from a `deploy-loki| target, so that we keep the deployment logic all via make targets?

@eranra
Copy link
Contributor Author

eranra commented Feb 21, 2022

@jotak when I asked in slack and you and @stleerh provided two solutions I said to myself that we need something agreed and common for testing and experience with the operator. In flowlogs2metrics we have something that looks like https://github.com/netobserv/flowlogs2metrics/blob/main/Makefile#L128 ... it also deploys Grafana and Prometheus. Once we get the work from @OlivierCazade to deploy the dependent operator, maybe the script to deploy Loki will not be needed.

As for the hacking doc ... it was very helpful to me when I did the end-to-end tests with the console and flowlogs2metrics deployed with the operator ... especially https://github.com/netobserv/network-observability-operator/pull/61/files#diff-88be51eadd2c2ccdf0a6f5bb8255bbe4f37f2b887a6839f638bc45f6cfe18bb4R12-R26 are missing and should be added somewhere.

Please advise what you prefer to have here ...
(1) Close this PR and just save things locally
(2) Keep part of the PR for Hacking and remove other parts
(3) Keep all ...

From my PoV ./... working with make run i.e. https://github.com/netobserv/network-observability-operator/pull/61/files#diff-88be51eadd2c2ccdf0a6f5bb8255bbe4f37f2b887a6839f638bc45f6cfe18bb4R20 is very important and efficient and I really think that this might help others when they start to work with the oprerator.

@jotak
Copy link
Member

jotak commented Feb 21, 2022

I like the approach you have in FL2M, it's similar to our "zero-click-loki" wired in the Makefile, I think we could have the same here. And same for grafana.

Also +1 for documenting the make run approach, but it could actually be in the main readme and not in a separate hack doc.

@jpinsonneau
Copy link
Contributor

+1 for make run

Also consider this new option for quick deploy #59
At the end, there is no need to manipulate anything else than the FlowCollector yaml

@eranra eranra changed the title hack: development with loki hack: development using "make run" (with loki) Feb 23, 2022
@eranra eranra force-pushed the hack_loki branch 3 times, most recently from a9a0d98 to 0fa6f1e Compare February 23, 2022 12:21
@jotak
Copy link
Member

jotak commented Feb 24, 2022

/lgtm

@memodi @jechen0648 there's a couple of changes in the Makefile here, not sure if it impacts some test cases / doc that you wrote. Not a big change, especially if you were using make create-sample you should change to make deploy-sample-cr once this PR is merged

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@stleerh
Copy link
Contributor

stleerh commented Mar 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 2, 2022
@jotak jotak added the no-qe This PR doesn't necessitate QE approval label Mar 2, 2022
@jotak
Copy link
Member

jotak commented Mar 2, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Mar 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

@openshift-ci openshift-ci bot added the approved label Mar 2, 2022
@openshift-merge-robot openshift-merge-robot merged commit bb4329d into netobserv:main Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm no-qe This PR doesn't necessitate QE approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants