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 regression on deploying latest #280

Merged
merged 2 commits into from
Mar 1, 2023
Merged

Fix regression on deploying latest #280

merged 2 commits into from
Mar 1, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Feb 28, 2023

In #274 the way IMG is used is changed, which broke deploying all latest components

This patch introduces a new "deploy-latest" target to explicitly deploy all components on their main branches

@jotak
Fix regression on deploying latest
c92f224 
In netobserv#274
the way IMG is used is changed, which broke deploying all latest
components

This patch introduces a new "deploy-latest" target to explicitly deploy
all components on their `main` branches
msherif1234
msherif1234 requested changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@msherif1234 msherif1234 left a comment

Choose a reason for hiding this comment

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

we need to reflect this new build target in the readme and developement doc files

Makefile Outdated
@@ -276,6 +276,10 @@ uninstall: kustomize ## Uninstall CRDs from the K8s cluster specified in ~/.kube

deploy: kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
cd config/manager && $(KUSTOMIZE) edit set image controller=${IMG}
$(KUSTOMIZE) build config/openshift | kubectl apply -f -

deploy-latest: kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config.
Copy link
Contributor

Choose a reason for hiding this comment

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

pls fix the comment here to indicate it will deploy all images with main tag

@msherif1234
Copy link
Contributor

so how we can deploy custom build operator image with latest of other components ?
assume u have local changes in the operator and u need to build and push and test with latest of agent/flb and others ?

maybe we can just check if IMG is set to different than default then use it for the operator and use the pinned versions for all others, if not set use latest for all
with just make deploy target

@memodi
Copy link
Contributor

memodi commented Feb 28, 2023

so how we can deploy custom build operator image with latest of other components ? assume u have local changes in the operator and u need to build and push and test with latest of agent/flb and others ?

@msherif1234 - I just tried this, deployed the image from PR #263 , I checkout that PR and used IMG that's build on that PR:

$ IMG="quay.io/netobserv/network-observability-operator:fea8ce2" make deploy
...
...
$ oc describe pod/$NOO | egrep "Image:|IMAGE"
    Image:         quay.io/netobserv/network-observability-operator:fea8ce2
      --ebpf-agent-image=$(RELATED_IMAGE_EBPF_AGENT)
      --flowlogs-pipeline-image=$(RELATED_IMAGE_FLOWLOGS_PIPELINE)
      --console-plugin-image=$(RELATED_IMAGE_CONSOLE_PLUGIN)
      RELATED_IMAGE_EBPF_AGENT:         quay.io/netobserv/netobserv-ebpf-agent:main
      RELATED_IMAGE_FLOWLOGS_PIPELINE:  quay.io/netobserv/flowlogs-pipeline:main
      RELATED_IMAGE_CONSOLE_PLUGIN:     quay.io/netobserv/network-observability-console-plugin:main
    Image:         gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0

memodi
memodi requested changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@memodi memodi left a comment

Choose a reason for hiding this comment

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

LGTM on the code, it does deploy NOO image tagged with "main" when using "deploy-latest" target. Could we document to set env variable for SED for MAC systems because those commands doesn't work when we use other than GNU sed.

@jotak
Copy link
Member Author

jotak commented Feb 28, 2023

Doc updated and should now work with gsed

@msherif1234
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 28, 2023
@Amoghrd
Copy link
Contributor

Amoghrd commented Feb 28, 2023

Verified with sed and gsed and it deploys the main image for the operator and its components.
/lgtm
Thanks!

@memodi
Copy link
Contributor

memodi commented Feb 28, 2023

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved QE has approved this pull request label Feb 28, 2023
@jotak
Copy link
Member Author

jotak commented Mar 1, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented Mar 1, 2023

[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 1, 2023
@openshift-merge-robot openshift-merge-robot merged commit 1b5925f into netobserv:main Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@memodi memodi memodi requested changes

@Amoghrd Amoghrd Awaiting requested review from Amoghrd

@msherif1234 msherif1234 Awaiting requested review from msherif1234

Assignees

@msherif1234 msherif1234

@Amoghrd Amoghrd

@memodi memodi

Labels
approved lgtm qe-approved QE has approved this pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jotak @msherif1234 @memodi @Amoghrd @openshift-merge-robot