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

NETOBSERV-956 Allow modifying namespace from web console #301

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

msherif1234
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #301 (d508e63) into main (44e1b37) will increase coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   49.66%   49.67%   +0.01%     
==========================================
  Files          43       43              
  Lines        5020     5075      +55     
==========================================
+ Hits         2493     2521      +28     
- Misses       2324     2343      +19     
- Partials      203      211       +8     
Flag Coverage Δ
unittests 49.67% <50.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/v1alpha1/flowcollector_types.go 100.00% <ø> (ø)
api/v1alpha1/flowcollector_webhook.go 0.00% <0.00%> (ø)
api/v1alpha1/zz_generated.conversion.go 0.27% <0.00%> (ø)
api/v1beta1/flowcollector_types.go 100.00% <ø> (ø)
api/v1beta1/zz_generated.deepcopy.go 42.33% <0.00%> (ø)
controllers/consoleplugin/consoleplugin_objects.go 92.45% <0.00%> (ø)
controllers/constants/constants.go 100.00% <ø> (ø)
pkg/helper/flowcollector.go 61.70% <50.00%> (ø)
controllers/flowlogspipeline/flp_common_objects.go 85.28% <68.42%> (+0.42%) ⬆️
controllers/flowlogspipeline/flp_reconciler.go 75.38% <78.57%> (+13.12%) ⬆️

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@msherif1234 msherif1234 changed the title WIP: Allow modifying namespace from web console Allow modifying namespace from web console Mar 21, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 21, 2023
@github-actions
Copy link

New images:

  • quay.io/netobserv/network-observability-operator:2d71da4
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-2d71da4
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-2d71da4

They will expire after two weeks.

Catalog source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: netobserv-dev
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-2d71da4
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

@jotak
Copy link
Member

jotak commented Mar 22, 2023

/lgtm

cc @OlivierCazade does that work for you too (wrt your PRs) ?

OlivierCazade
OlivierCazade previously approved these changes Mar 22, 2023
Copy link
Contributor

@OlivierCazade OlivierCazade left a comment

Choose a reason for hiding this comment

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

LGTM yes. We will just have to update the namespace name in the servicemonitor.

@msherif1234
Copy link
Contributor Author

Hi @OlivierCazade is there a way to make sure this all labels we need before this get merged ?

@OlivierCazade
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot removed the lgtm label Mar 22, 2023
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 22, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 22, 2023
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 22, 2023
@msherif1234
Copy link
Contributor Author

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 22, 2023
@github-actions
Copy link

New images:

  • quay.io/netobserv/network-observability-operator:db95593
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-db95593
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-db95593

They will expire after two weeks.

Catalog source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: netobserv-dev
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-db95593
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

cf @memodi comments.
I can take care of doing the midstream work, and let you review

@openshift-ci openshift-ci bot removed the lgtm label Mar 24, 2023
@@ -13,7 +13,7 @@ spec:
scheme: https
tlsConfig:
caFile: /etc/prometheus/configmaps/serving-certs-ca-bundle/service-ca.crt
serverName: netobserv-metrics-service.openshift-operators.svc
serverName: netobserv-metrics-service.openshift-netobserv-operator.svc
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
serverName: netobserv-metrics-service.openshift-netobserv-operator.svc
serverName: netobserv-metrics-service.netobserv-operator.svc

Should we use netobserv-operator upstream and openshift-netobserv-operator downstream ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are currently using openshift-operaotors for both so this PR does not change it.

This would force us to maintain a third kind of deployment in the configuration, we already have openshift and kubernetes

@@ -336,6 +336,8 @@ metadata:
containerImage: quay.io/netobserv/network-observability-operator:1.0.2
createdAt: ':created-at:'
description: Network flows collector and monitoring solution
operatorframework.io/cluster-monitoring: "true"
operatorframework.io/suggested-namespace: openshift-netobserv-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
operatorframework.io/suggested-namespace: openshift-netobserv-operator
operatorframework.io/suggested-namespace: netobserv-operator

@@ -9,6 +9,8 @@ metadata:
containerImage: ':container-image:'
createdAt: ':created-at:'
description: Network flows collector and monitoring solution
operatorframework.io/cluster-monitoring: "true"
operatorframework.io/suggested-namespace: openshift-netobserv-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
operatorframework.io/suggested-namespace: openshift-netobserv-operator
operatorframework.io/suggested-namespace: netobserv-operator

Signed-off-by: msherif1234 <mmahmoud@redhat.com>
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 24, 2023
@msherif1234
Copy link
Contributor Author

msherif1234 commented Mar 24, 2023

https://gitlab.cee.redhat.com/netobserv-midstream/network-observability-operator-cpaas/-/merge_requests/145/diffs will be used to add csv namesspaces annotation for monitoring

@jotak
Copy link
Member

jotak commented Mar 24, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 24, 2023
@skrthomas
Copy link
Contributor

Noticed below while testing for this change: If openshift-netobserv-operator is left behind from previous installation , next time when you try to install Operator checkbox to enable metrics does not appear.

couple of thoughts:

1. Should we delete `openshift-netobserv-operator` NS as part of uninstallation automatically handled by Operator? OR

2. Should we add a step to delete `openshift-netobserv-operator` NS as part of uninstallation documentation procedure?

cc @skrthomas

@msherif1234 did the OLM team have any insight as to what we should do wrt taking care of ns deletion? It can be added as an uninstallation step cc @JoeAldinger

@memodi
Copy link
Contributor

memodi commented Mar 24, 2023

/ok-to-test

@openshift-ci openshift-ci bot added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Mar 24, 2023
@github-actions
Copy link

New images:

  • quay.io/netobserv/network-observability-operator:18ea3a7
  • quay.io/netobserv/network-observability-operator-bundle:v0.0.0-18ea3a7
  • quay.io/netobserv/network-observability-operator-catalog:v0.0.0-18ea3a7

They will expire after two weeks.

Catalog source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: netobserv-dev
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/netobserv/network-observability-operator-catalog:v0.0.0-18ea3a7
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

@msherif1234
Copy link
Contributor Author

Noticed below while testing for this change: If openshift-netobserv-operator is left behind from previous installation , next time when you try to install Operator checkbox to enable metrics does not appear.
couple of thoughts:

1. Should we delete `openshift-netobserv-operator` NS as part of uninstallation automatically handled by Operator? OR

2. Should we add a step to delete `openshift-netobserv-operator` NS as part of uninstallation documentation procedure?

cc @skrthomas

@msherif1234 did the OLM team have any insight as to what we should do wrt taking care of ns deletion? It can be added as an uninstallation step cc @JoeAldinger

@skrthomas yes we need to doc deleting NS as part of the uninstall process we discussed this on slack and here is the thread for reference https://redhat-internal.slack.com/archives/C02939DP5L5/p1679502364770879?thread_ts=1678975816.191759&cid=C02939DP5L5

@memodi
Copy link
Contributor

memodi commented Mar 24, 2023

/label qe-approved

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

jotak commented Mar 27, 2023

/approve

thanks @msherif1234 ! I'll cherry-pick to 1.2

@openshift-ci
Copy link

openshift-ci bot commented Mar 27, 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-merge-robot openshift-merge-robot merged commit 3130154 into netobserv:main Mar 27, 2023
jotak pushed a commit that referenced this pull request Mar 27, 2023
Signed-off-by: msherif1234 <mmahmoud@redhat.com>
@jotak jotak changed the title Allow modifying namespace from web console NETOBSERV-956 Allow modifying namespace from web console Mar 27, 2023
@JoeAldinger
Copy link

This PR is being reverted because it was merged early and will go out when the Operator goes out.

@jotak
Copy link
Member

jotak commented Mar 30, 2023

@JoeAldinger I'm not sure to get your comment. Are you referring to the related documentation PR, not this very PR?

( openshift/openshift-docs#57468 )

@JoeAldinger
Copy link

JoeAldinger commented Mar 30, 2023

@JoeAldinger I'm not sure to get your comment. Are you referring to the related documentation PR, not this very PR?

( openshift/openshift-docs#57468 )

Sorry, I was in panic mode last night when I realized I had things merged on the docs side that shouldn't have been and accidentally commented on the wrong PR. PR #57748 is the PR that was reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. qe-approved QE has approved this pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants