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

Implement CLI / Config with Cobra / Viper #1043

Merged
merged 32 commits into from
Jul 31, 2024

Conversation

resoluteCoder
Copy link
Contributor

@resoluteCoder resoluteCoder commented May 16, 2024

The Problem

Currently Receptor uses a custom config parser that is tightly coupled as the parser not only reads the config file, but also initializes the relevant parts of the project. This custom parser is also not idiomatic, making it more difficult to understand the project.
The custom parser is difficult to extend, this can be seen in the reluctance to add more config fields, instead relying on environment variables.

The solution

This change proposes a backwards compatible way to move away from relying on the old cli and using cobra/viper to implement a version 2.

Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

cmd/config.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
cmd/config.go Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 37.38318% with 67 lines in your changes missing coverage. Please review.

Project coverage is 43.04%. Comparing base (1cffdce) to head (10745fe).
Report is 74 commits behind head on devel.

Files with missing lines Patch % Lines
pkg/workceptor/kubernetes.go 53.84% 17 Missing and 1 partial ⚠️
pkg/backends/null.go 0.00% 13 Missing ⚠️
pkg/backends/udp.go 25.00% 5 Missing and 1 partial ⚠️
pkg/backends/websockets.go 25.00% 5 Missing and 1 partial ⚠️
pkg/certificates/cli.go 40.00% 5 Missing and 1 partial ⚠️
pkg/controlsvc/controlsvc.go 33.33% 3 Missing and 1 partial ⚠️
pkg/logger/logger.go 42.85% 3 Missing and 1 partial ⚠️
pkg/workceptor/python.go 33.33% 3 Missing and 1 partial ⚠️
pkg/backends/tcp.go 33.33% 1 Missing and 1 partial ⚠️
pkg/netceptor/tlsconfig.go 33.33% 1 Missing and 1 partial ⚠️
... and 1 more
@@            Coverage Diff             @@
##            devel    #1043      +/-   ##
==========================================
+ Coverage   42.47%   43.04%   +0.56%     
==========================================
  Files          43       47       +4     
  Lines        6775     7430     +655     
==========================================
+ Hits         2878     3198     +320     
- Misses       3666     3994     +328     
- Partials      231      238       +7     
Files with missing lines Coverage Δ
pkg/backends/tcp.go 3.15% <33.33%> (+0.48%) ⬆️
pkg/netceptor/tlsconfig.go 60.31% <33.33%> (-0.66%) ⬇️
pkg/workceptor/command.go 49.61% <50.00%> (-0.19%) ⬇️
pkg/controlsvc/controlsvc.go 77.89% <33.33%> (-0.48%) ⬇️
pkg/logger/logger.go 20.30% <42.85%> (+0.30%) ⬆️
pkg/workceptor/python.go 10.81% <33.33%> (+1.98%) ⬆️
pkg/backends/udp.go 2.87% <25.00%> (+0.44%) ⬆️
pkg/backends/websockets.go 55.08% <25.00%> (-1.14%) ⬇️
pkg/certificates/cli.go 29.93% <40.00%> (+0.07%) ⬆️
pkg/backends/null.go 0.00% <0.00%> (ø)
... and 1 more

... and 6 files with indirect coverage changes

Components Coverage Δ
Go 42.51% <37.38%> (+0.03%) ⬆️
Receptorctl 49.31% <ø> (∅)

@resoluteCoder resoluteCoder marked this pull request as ready for review July 5, 2024 23:02
docs/source/conf.py Outdated Show resolved Hide resolved
@AaronH88
Copy link
Contributor

Overall looking good,

Can I get some example of the trace option please as I cant get it working locally 😄

Copy link
Member

@shanemcd shanemcd left a comment

Choose a reason for hiding this comment

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

Please add a description to this PR or create an associated issue with more information. For a PR this big / important, we should summarize what we're doing here and the approach we're taking. Here's a couple examples for you to follow:

docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/getting_started_guide/index.rst Outdated Show resolved Hide resolved
docs/source/user_guide/basic_usage.rst Outdated Show resolved Hide resolved
docs/source/user_guide/basic_usage.rst Outdated Show resolved Hide resolved
docs/source/user_guide/connecting_nodes.rst Outdated Show resolved Hide resolved
docs/source/user_guide/tls.rst Outdated Show resolved Hide resolved
pkg/types/main.go Outdated Show resolved Hide resolved
pkg/types/main.go Outdated Show resolved Hide resolved
pkg/types/main.go Outdated Show resolved Hide resolved
pkg/types/main.go Outdated Show resolved Hide resolved
@AaronH88
Copy link
Contributor

Overall looking good,

Can I get some example of the trace option please as I cant get it working locally 😄

Trace seems to be strange even on the original and dosent do what I expected at all.
Lets create something in our backlog for using a different and better logging tool where tracing can be turned off and on at run time.

Feel free to ignore this trace comment and this PR looks OK to me 😄

Copy link
Contributor

@AaronH88 AaronH88 left a comment

Choose a reason for hiding this comment

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

lgtm,

Can we resolve the conflicts then we are good to merge 😄

Copy link

@AaronH88 AaronH88 merged commit ac7d4be into ansible:devel Jul 31, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants