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

Enum replacement in API #614

Merged
merged 4 commits into from
Mar 22, 2024
Merged

Enum replacement in API #614

merged 4 commits into from
Mar 22, 2024

Conversation

jotak
Copy link
Member

@jotak jotak commented Feb 26, 2024 •
edited

Fixes #608
All enums are moved to the new enum model

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 26, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@codecov Codecov
Copy link

codecov bot commented Feb 26, 2024 •
edited

Codecov Report

Attention: Patch coverage is 83.82353% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 67.38%. Comparing base (b1ccc26) to head (10410f7).

Files Patch % Lines
pkg/pipeline/encode/encode_kafka.go 0.00% 5 Missing ⚠️
pkg/pipeline/transform/transform_network.go 66.66% 5 Missing ⚠️
...ipeline/encode/opentelemetry/encode_otlpmetrics.go 33.33% 4 Missing ⚠️
pkg/pipeline/encode/encode_prom_metric.go 50.00% 3 Missing ⚠️
pkg/pipeline/utils/sasl.go 0.00% 2 Missing ⚠️
pkg/api/conntrack.go 90.90% 1 Missing ⚠️
pkg/pipeline/decode/decode.go 50.00% 1 Missing ⚠️
pkg/pipeline/ingest/ingest_stdin.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #614      +/-   ##
==========================================
- Coverage   67.44%   67.38%   -0.06%     
==========================================
  Files         110      102       -8     
  Lines        7633     7589      -44     
==========================================
- Hits         5148     5114      -34     
+ Misses       2173     2163      -10     
  Partials      312      312
Flag Coverage Δ
unittests 67.38% <83.82%> (-0.06%) ⬇️

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.

@jotak jotak force-pushed the enums branch 2 times, most recently from 7048e4d to c4273c3 Compare February 26, 2024 14:40
@jotak jotak mentioned this pull request Mar 7, 2024
Closed
jotak
jotak commented Mar 18, 2024
View reviewed changes
pkg/pipeline/encode/opentelemetry/encode_otlpmetrics.go
@@ -168,7 +168,7 @@ func NewEncodeOtlpMetrics(opMetrics *operational.Metrics, params config.StagePar
return nil, err
}
metricCommon.AddHist(histo, mInfo)
case "default":
Copy link
Member Author

@jotak jotak Mar 18, 2024 •
edited

Choose a reason for hiding this comment

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

example of lack of type safety with the previous model: "default" has never been an allowed enum here. Now this would be caught at compile time.

@jotak jotak marked this pull request as ready for review March 18, 2024 12:52
@jotak
Enum replacement in API
a1daf1a 
First step / PoC for fixing netobserv#608
Starting with metrics filters only; other enums should follow
@jotak
simplify sed for docgen
f2532be
@jotak
Complete using new enum types
5a49990
OlivierCazade
OlivierCazade approved these changes Mar 21, 2024
View reviewed changes
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

@openshift-ci openshift-ci bot added the lgtm label Mar 21, 2024
@jotak jotak added no-qe This PR doesn't necessitate QE approval no-doc This PR doesn't require documentation change on the NetObserv operator ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. labels Mar 22, 2024
@github-actions GitHub Actions
Copy link

New image:
quay.io/netobserv/flowlogs-pipeline:4f8630a

It will expire after two weeks.

To deploy this build, run from the operator repo, assuming the operator is running:

USER=netobserv VERSION=4f8630a make set-flp-image

@jotak
Copy link
Member Author

jotak commented Mar 22, 2024

/approve

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Mar 22, 2024

[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-bot openshift-merge-bot bot merged commit d2b2352 into netobserv:main Mar 22, 2024
14 checks passed
@jotak jotak added the breaking-change This pull request has breaking changes. They should be described in PR description. label Mar 22, 2024
@jotak
Copy link
Member Author

jotak commented Mar 22, 2024

no big deal, but adding breaking change label due to this commit 10410f7 - it was integrated in this PR by mistake, I was expecting to bring it later via #640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlivierCazade OlivierCazade OlivierCazade approved these changes

@KalmanMeth KalmanMeth Awaiting requested review from KalmanMeth

@jpinsonneau jpinsonneau Awaiting requested review from jpinsonneau

Assignees

@OlivierCazade OlivierCazade

Labels
approved breaking-change This pull request has breaking changes. They should be described in PR description. lgtm no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval ok-to-test To set manually when a PR is safe to test. Triggers image build on PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some issues with enums management in API
3 participants
@jotak @OlivierCazade @openshift-merge-robot