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

feat: tools-2777 associate validation errors with the config section that caused… #38

Merged
merged 10 commits into from
Apr 2, 2024

Conversation

dwelch-spike
Copy link
Contributor

@dwelch-spike dwelch-spike commented Mar 27, 2024

… them
Note: tools-2885 was found while working on this PR so I fixed it in c0dfad6 . The issue caused the output of the validate command to go to stderr.

I ended up backing the tools-2885 changes out.

@dwelch-spike dwelch-spike self-assigned this Mar 27, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2024

Codecov Report

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

Project coverage is 78.79%. Comparing base (d694a4a) to head (c0dfad6).
Report is 1 commits behind head on main.

Files Patch % Lines
conf/config_validator.go 91.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
+ Coverage   77.87%   78.79%   +0.91%     
==========================================
  Files          14       14              
  Lines         940     1009      +69     
==========================================
+ Hits          732      795      +63     
- Misses        141      145       +4     
- Partials       67       69       +2     

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

cmd/root.go Outdated Show resolved Hide resolved
conf/config_validator.go Show resolved Hide resolved
@dwelch-spike
Copy link
Contributor Author

I found that the validation failures the validate command prints were actually going to stderr not stdout. They should be going to stdout. I filed a bug ticket https://aerospike.atlassian.net/browse/TOOLS-2885, since it was a 1 line change I fixed the issue in c0dfad6 . Just changing cmd.Print to fmt.Print does the trick.

Copy link

@jdogmcsteezy jdogmcsteezy left a comment

Choose a reason for hiding this comment

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

LGTM after TOOLS-2885 is backed out

@dwelch-spike
Copy link
Contributor Author

tools-2885 is backed out, waiting for tests to pass then will merge

@dwelch-spike dwelch-spike merged commit 018eb7b into main Apr 2, 2024
5 checks passed
@dwelch-spike dwelch-spike deleted the tools-2777 branch April 2, 2024 20:20
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.

3 participants