NETOBSERV: 1374 Make enums camelCase #483

Amoghrd · 2023-11-03T16:43:44Z

Description

Dependencies

n/a

Checklist

If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.

codecov · 2023-11-03T17:46:42Z

Codecov Report

Attention: 123 lines in your changes are missing coverage. Please review.

Comparison is base (74a4814) 62.26% compared to head (0d51d44) 61.92%.
Report is 14 commits behind head on main.

❗ Current head 0d51d44 differs from pull request most recent head 64b8e54. Consider uploading reports for the commit 64b8e54 to get more accurate results

Files	Patch %	Lines
api/v1beta1/flowcollector_webhook.go	37.03%	52 Missing and 16 partials ⚠️
api/v1beta1/zz_generated.conversion.go	11.29%	55 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
- Coverage   62.26%   61.92%   -0.35%     
==========================================
  Files          55       55              
  Lines        6769     6875     +106     
==========================================
+ Hits         4215     4257      +42     
- Misses       2238     2286      +48     
- Partials      316      332      +16

Flag	Coverage Δ
unittests	`61.92% <40.29%> (-0.35%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Amoghrd · 2023-11-03T21:12:15Z

@jpinsonneau @msherif1234 @jotak I am getting errors when I do make bundle, could you guys PTAL and let me know what needs to be done here?

make bundle
/Users/amoghrd/repos/netobserv/network-observability-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
/Users/amoghrd/repos/netobserv/network-observability-operator/bin/controller-gen "crd:trivialVersions=true,preserveUnknownFields=false" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
/Users/amoghrd/repos/netobserv/network-observability-operator/bin/crdoc --resources config/crd/bases/flows.netobserv.io_flowcollectors.yaml --output docs/FlowCollector.md
GOBIN=/Users/amoghrd/repos/netobserv/network-observability-operator/bin ./scripts/go_install.sh k8s.io/code-generator/cmd/conversion-gen conversion-gen v0.25.0
/Library/Developer/CommandLineTools/usr/bin/make clean-generated-conversions SRC_DIRS="./api/v1alpha1,./api/v1beta1"
(IFS=','; for i in ./api/v1alpha1,./api/v1beta1; do find $i -type f -name 'zz_generated.conversion*' -exec rm -f {} ;; done)
/Users/amoghrd/repos/netobserv/network-observability-operator/bin/conversion-gen
--input-dirs=./api/v1alpha1
--input-dirs=./api/v1beta1
--build-tag=ignore_autogenerated_core
--output-file-base=zz_generated.conversion
--output-base=/Users/amoghrd/repos/netobserv/network-observability-operator
--go-header-file=./hack/boilerplate/boilerplate.generatego.txt
E1103 17:00:54.972681 4214 conversion.go:193] Rename function ./api/v1alpha1 Convert_v1beta2_FlowCollectorEBPF_To_v1alpha1_FlowCollectorEBPF -> Convert_v1beta2_FlowCollectorEbpf_To_v1alpha1_FlowCollectorEBPF to match expected conversion signature
E1103 17:00:54.999611 4214 conversion.go:756] Warning: could not find nor generate a final Conversion function for ./api/v1alpha1.FlowCollectorAgent -> github.com/netobserv/network-observability-operator/api/v1beta2.FlowCollectorAgent
E1103 17:00:54.999636 4214 conversion.go:757] the following fields need manual conversion:
E1103 17:00:54.999641 4214 conversion.go:759] - IPFIX
E1103 17:00:54.999644 4214 conversion.go:759] - EBPF
E1103 17:00:54.999706 4214 conversion.go:756] Warning: could not find nor generate a final Conversion function for github.com/netobserv/network-observability-operator/api/v1beta2.FlowCollectorAgent -> ./api/v1alpha1.FlowCollectorAgent
E1103 17:00:54.999713 4214 conversion.go:757] the following fields need manual conversion:
E1103 17:00:54.999717 4214 conversion.go:759] - Ipfix
E1103 17:00:54.999720 4214 conversion.go:759] - Ebpf
E1103 17:00:55.124730 4214 conversion.go:756] Warning: could not find nor generate a final Conversion function for ./api/v1beta1.FlowCollectorAgent -> github.com/netobserv/network-observability-operator/api/v1beta2.FlowCollectorAgent
E1103 17:00:55.124745 4214 conversion.go:757] the following fields need manual conversion:
E1103 17:00:55.124750 4214 conversion.go:759] - IPFIX
E1103 17:00:55.124753 4214 conversion.go:759] - EBPF
E1103 17:00:55.124819 4214 conversion.go:756] Warning: could not find nor generate a final Conversion function for github.com/netobserv/network-observability-operator/api/v1beta2.FlowCollectorAgent -> ./api/v1beta1.FlowCollectorAgent
E1103 17:00:55.124826 4214 conversion.go:757] the following fields need manual conversion:
E1103 17:00:55.124829 4214 conversion.go:759] - Ipfix
E1103 17:00:55.124833 4214 conversion.go:759] - Ebpf
cat ./config/crd/bases/flows.netobserv.io_flowcollectors.yaml
| ./bin/yq eval-all
'(.spec.versions.[]|select(.name != "v1beta2").storage) = false,(.spec.versions.[]|select(.name == "v1beta2").storage) = true'
> ./hack/cloned.flows.netobserv.io_flowcollectors.yaml
cd config/manager && /Users/amoghrd/repos/netobserv/network-observability-operator/bin/kustomize edit set image controller=quay.io/amoghrd/network-observability-operator:main
gsed -i -r 'sebpf-agent:.+~~ebpf-agent:v0.3.2~~' ./config/manager/manager.yaml
gsed -i -r 'sflowlogs-pipeline:.+~~flowlogs-pipeline:v0.1.10~~' ./config/manager/manager.yaml
gsed -i -r 'sconsole-plugin:.+~~console-plugin:v0.1.11~~' ./config/manager/manager.yaml
gsed -i -r 'snetwork-observability-operator/blob/[^/]+/~network-observability-operator/blob/main/g' ./config/csv/bases/netobserv-operator.clusterserviceversion.yaml
gsed -i -r 'snetwork-observability-operator/blob/[^/]+/~network-observability-operator/blob/main/g' ./config/descriptions/upstream.md
gsed -i -r 'snetwork-observability-operator/blob/[^/]+/network-observability-operator/blob/main/g' ./config/descriptions/ocp.md
gsed -i -r 'sreplaces: netobserv-operator.v.*~~replaces: netobserv-operator.v1.0.3~~' ./config/csv/bases/netobserv-operator.clusterserviceversion.yaml
rm -r bundle/manifests
rm -r bundle/metadata
gsed -e 's/^/ /' config/descriptions/upstream.md > tmp-desc
/Users/amoghrd/repos/netobserv/network-observability-operator/bin/kustomize build config/openshift-olm
| gsed -e 's:container-image:~~quay.io/amoghrd/network-observability-operator:main~~'
| gsed -e "/':full-description:'/r tmp-desc"
| gsed -e "s/':full-description:'/|-/"
| ./bin/operator-sdk generate bundle -q --overwrite --version 1.0.4 --channels=latest,v1.0.x --default-channel=latest
INFO[0001] Creating bundle.Dockerfile
INFO[0001] Creating bundle/metadata/annotations.yaml
INFO[0001] Bundle metadata generated successfully
rm tmp-desc
sh -c '
VALIDATION_OUTPUT=$(./bin/operator-sdk bundle validate ./bundle --select-optional suite=operatorframework);
echo ${VALIDATION_OUTPUT};
if [ $(echo ${VALIDATION_OUTPUT} | grep -i 'warning' | wc -c) -gt 0 ]; then echo "please correct warnings and errors first"; exit -1 ; fi
'
INFO[0000] All validation tests have completed successfully

jpinsonneau · 2023-11-06T08:53:59Z

Hey @Amoghrd, thanks for contributing with us 😸

You should not update golang structs here, only enums values. Else the linter will complain.
Since we are changing enum values, we will need to add some conversions between the uppercase / camel case api versions. You can refresh go-conversions using make generate
After that, you will only need to run make update-bundle

jpinsonneau · 2023-11-06T13:56:11Z

api/v1alpha1/flowcollector_types.go

+	// +k8s:conversion-gen=false
 	Exporters []*FlowCollectorExporter `json:"exporters"`


I have to skip conversion-gen here since ExporterType doesn't overlap anymore

jpinsonneau · 2023-11-06T13:59:36Z

pkg/conversion/conversion.go

+// Following K8S convention, mixed capitalization should be preserved
+// see https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#constants
+var upperPascalExceptions = map[string]string{
+	"IPFIX":        "IPFIX",
+	"EBPF":         "eBPF",
+	"SCRAM-SHA512": "ScramSHA512",
+}


Managing exceptions manually here. eBPF and IPFIX are kept as their original acronyms case.

Following discussion with @jotak, he suggested to use ScramSHA512 for better readability.
In AMQ-Streams, this is fully lowercase as scram-sha-512 😞 We may consider SCRAM-SHA-512 as alternative.

jotak · 2023-11-10T08:03:26Z

pkg/conversion/conversion.go

+
+	// Split on '-' or '_' rune, capitalize first letter of each part and join them
+	var sb strings.Builder
+	array := regexp.MustCompile(`[\-\_]+`).Split(strings.ToLower(str), -1)


Perhaps pre-compute the regex? That's generally a good practice and won't change your design. Like this on package scope:

var upperTokenizer = regexp.MustCompile(`[\-\_]+`)

(You may find me nitpicking but I think just a static map of that dozen of converted strings would have done the job without the need for any compute like this - it's called at every read/write of the CR, at least as long as v1beta1 is still the stored version. If they were many more strings to convert I'd agree that a map wouldn't be the best choice. Anyway that's not a huge compute and conversions don't happen so often so I'm fine with this option as well.)

sure, done in 0d51d44

jotak · 2023-11-10T08:07:54Z

A small nit, other than that lgtm; note there's a lint failure => missing a nolint annotation on one of the webhook conversion func

jpinsonneau · 2023-11-10T09:22:50Z

A small nit, other than that lgtm; note there's a lint failure => missing a nolint annotation on one of the webhook conversion func

I've rebased the PR to fix these. We should be good to go now 😸
cc @Amoghrd

github-actions · 2023-11-10T09:26:04Z

New images:

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

They will expire after two weeks.

To deploy this build:

# Direct deployment, from operator repo
IMAGE=quay.io/netobserv/network-observability-operator:f543ef2 make deploy

# Or using operator-sdk
operator-sdk run bundle quay.io/netobserv/network-observability-operator-bundle:v0.0.0-f543ef2

Or as a 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-f543ef2
  displayName: NetObserv development catalog
  publisher: Me
  updateStrategy:
    registryPoll:
      interval: 1m

jpinsonneau · 2023-11-13T12:19:15Z

@nathan-weinberg marking this PR as breaking tests since enums has been renamed.
Please let me know when we are good to merge

openshift-merge-robot · 2023-11-30T12:15:33Z

PR needs rebase.

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.

* NETOBSERV-1326: NETOBSERV-1231: Drops & RTT metrics - Added metrics: node_rtt, namespace_rtt, workload_rtt, node_drop_packets_total, node_drop_bytes_total, namespace_drop_packets_total, namespace_drop_bytes_total, workload_drop_packets_total, workload_drop_bytes_total - Add dashboards for drops (not yet for RTT, need to handle histomgrams in dashboards first) * Update CRD doc and tests with added metrics * Set new defaults * Update CRD doc * externalize metrics doc

Bumps [github.com/onsi/gomega](https://github.com/onsi/gomega) from 1.29.0 to 1.30.0. - [Release notes](https://github.com/onsi/gomega/releases) - [Changelog](https://github.com/onsi/gomega/blob/master/CHANGELOG.md) - [Commits](onsi/gomega@v1.29.0...v1.30.0) --- updated-dependencies: - dependency-name: github.com/onsi/gomega dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…etobserv#451) Co-authored-by: Joel Takvorian <jtakvori@redhat.com>

Bumps [github.com/onsi/ginkgo/v2](https://github.com/onsi/ginkgo) from 2.13.0 to 2.13.1. - [Release notes](https://github.com/onsi/ginkgo/releases) - [Changelog](https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md) - [Commits](onsi/ginkgo@v2.13.0...v2.13.1) --- updated-dependencies: - dependency-name: github.com/onsi/ginkgo/v2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…toring (netobserv#492) Bumps [github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring](https://github.com/prometheus-operator/prometheus-operator) from 0.68.0 to 0.69.1. - [Release notes](https://github.com/prometheus-operator/prometheus-operator/releases) - [Changelog](https://github.com/prometheus-operator/prometheus-operator/blob/main/CHANGELOG.md) - [Commits](prometheus-operator/prometheus-operator@v0.68.0...v0.69.1) --- updated-dependencies: - dependency-name: github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

openshift-ci · 2023-12-07T15:39:06Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2023-12-07T15:39:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jotak. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jpinsonneau · 2023-12-08T09:53:18Z

Closing this PR in favor of #511

I don't know why I was not able to force push in this PR:

 ! [remote rejected]   HEAD -> pr/Amoghrd/483 (permission denied)
error: failed to push some refs to 'https://github.com/amoghrd/network-observability-operator'

openshift-ci bot added the do-not-merge/work-in-progress label Nov 3, 2023

Amoghrd force-pushed the netobserv-1374 branch 2 times, most recently from 50f6abf to c2e131d Compare November 3, 2023 17:41

Amoghrd force-pushed the netobserv-1374 branch from c2e131d to 813f324 Compare November 3, 2023 17:49

Amoghrd changed the title ~~WIP: Update logTypes enum~~ WIP: NETOBSERV: 1374 Update logTypes enum Nov 3, 2023

Amoghrd changed the title ~~WIP: NETOBSERV: 1374 Update logTypes enum~~ NETOBSERV: 1374 Update logTypes enum Nov 3, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 3, 2023

jpinsonneau requested review from jotak, msherif1234 and OlivierCazade November 6, 2023 13:55

jpinsonneau reviewed Nov 6, 2023

View reviewed changes

Amoghrd changed the title ~~NETOBSERV: 1374 Update logTypes enum~~ NETOBSERV: 1374 Make enums camelCase Nov 6, 2023

Amoghrd requested a review from jpinsonneau November 7, 2023 14:13

jotak reviewed Nov 10, 2023

View reviewed changes

Amoghrd and others added 7 commits November 10, 2023 10:04

Update logTypes enum

24c7826

Update camelCase for other fields

8dc2ce3

Revert URL changes

7b65b1a

manage conversions

7409f53

Update eBPF enum

0d73c3e

Update ebpf enum 2

3070fc9

pre compute regex feedback

0d51d44

jpinsonneau force-pushed the netobserv-1374 branch from d03261d to 0d51d44 Compare November 10, 2023 09:20

jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Nov 10, 2023

jotak previously approved these changes Nov 10, 2023

View reviewed changes

openshift-ci bot assigned jotak Nov 10, 2023

openshift-ci bot added the lgtm label Nov 10, 2023

jpinsonneau added the testing-breaking-change label Nov 13, 2023

openshift-merge-robot added the needs-rebase label Nov 30, 2023

jpinsonneau and others added 8 commits December 7, 2023 10:30

update config (netobserv#488)

a42d42b

NETOBSERV-1340: Add operator to "Networking" category in OperatorHub (n…

a33de9b

…etobserv#451) Co-authored-by: Joel Takvorian <jtakvori@redhat.com>

Update camelCase for other fields

38dba7e

Revert URL changes

9af63d5

Amoghrd dismissed jotak’s stale review via 9af63d5 December 7, 2023 15:39

openshift-ci bot removed the lgtm label Dec 7, 2023

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Dec 7, 2023

Update camelCase for other fields

64b8e54

jpinsonneau mentioned this pull request Dec 8, 2023

NETOBSERV: 1374 Make enums pascal case #511

Merged

10 tasks

jpinsonneau closed this Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV: 1374 Make enums camelCase #483

NETOBSERV: 1374 Make enums camelCase #483

Amoghrd commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

Amoghrd commented Nov 3, 2023

jpinsonneau commented Nov 6, 2023 •

edited

Loading

jpinsonneau Nov 6, 2023

jpinsonneau Nov 6, 2023

jotak Nov 10, 2023

jpinsonneau Nov 10, 2023

jotak commented Nov 10, 2023

jpinsonneau commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

jpinsonneau commented Nov 13, 2023

openshift-merge-robot commented Nov 30, 2023

openshift-ci bot commented Dec 7, 2023

openshift-ci bot commented Dec 7, 2023

jpinsonneau commented Dec 8, 2023

		// +k8s:conversion-gen=false
		Exporters []*FlowCollectorExporter `json:"exporters"`

NETOBSERV: 1374 Make enums camelCase #483

NETOBSERV: 1374 Make enums camelCase #483

Conversation

Amoghrd commented Nov 3, 2023 • edited Loading

Description

Dependencies

Checklist

codecov bot commented Nov 3, 2023 • edited Loading

Codecov Report

Amoghrd commented Nov 3, 2023

jpinsonneau commented Nov 6, 2023 • edited Loading

jpinsonneau Nov 6, 2023

Choose a reason for hiding this comment

jpinsonneau Nov 6, 2023

Choose a reason for hiding this comment

jotak Nov 10, 2023

Choose a reason for hiding this comment

jpinsonneau Nov 10, 2023

Choose a reason for hiding this comment

jotak commented Nov 10, 2023

jpinsonneau commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

jpinsonneau commented Nov 13, 2023

openshift-merge-robot commented Nov 30, 2023

openshift-ci bot commented Dec 7, 2023

openshift-ci bot commented Dec 7, 2023

jpinsonneau commented Dec 8, 2023

Amoghrd commented Nov 3, 2023 •

edited

Loading

codecov bot commented Nov 3, 2023 •

edited

Loading

jpinsonneau commented Nov 6, 2023 •

edited

Loading