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

refactor: Use errors.Is() instead of direct comparison. #581

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

SahibYar
Copy link
Contributor

Comparing errors using equality operator is no longer a best practice. (Reference)

Testing

I executed make all after the changes, and here is the output.

go-fastly % make all

==> Downloading Go module
==> Downloading development dependencies
==> Downloading https://formulae.brew.sh/api/formula.jws.json
==> Downloading https://formulae.brew.sh/api/cask.jws.json
Warning: semgrep 1.103.0 is already installed and up-to-date.
To reinstall 1.103.0, run:
  brew reinstall semgrep
==> Tidying module
==> Running gofmt
==> Fixing imports
==> Testing go-fastly
?       github.com/fastly/go-fastly/v9/fastly/domains   [no test files]
?       github.com/fastly/go-fastly/v9/fastly/products  [no test files]
?       github.com/fastly/go-fastly/v9/internal/test_utils      [no test files]
ok      github.com/fastly/go-fastly/v9/fastly   2.295s
ok      github.com/fastly/go-fastly/v9/fastly/domains/v1        4.335s
ok      github.com/fastly/go-fastly/v9/fastly/image_optimizer_default_settings  2.191s
ok      github.com/fastly/go-fastly/v9/fastly/products/bot_management   3.062s
ok      github.com/fastly/go-fastly/v9/fastly/products/brotli_compression       1.761s
ok      github.com/fastly/go-fastly/v9/fastly/products/ddos_protection  1.324s
ok      github.com/fastly/go-fastly/v9/fastly/products/domain_inspector 0.902s
ok      github.com/fastly/go-fastly/v9/fastly/products/fanout   2.625s
ok      github.com/fastly/go-fastly/v9/fastly/products/image_optimizer  3.503s
ok      github.com/fastly/go-fastly/v9/fastly/products/log_explorer_insights    3.920s
ok      github.com/fastly/go-fastly/v9/fastly/products/ngwaf    4.539s
ok      github.com/fastly/go-fastly/v9/fastly/products/origin_inspector 4.931s
ok      github.com/fastly/go-fastly/v9/fastly/products/websockets       4.529s
ok      github.com/fastly/go-fastly/v9/internal/productcore     4.594s
==> Running go vet
==> Running staticcheck
staticcheck 2023.1.3 (v0.4.3)
if command -v semgrep &> /dev/null; then semgrep ci --config auto --exclude-rule generic.secrets.security.detected-private-key.detected-private-key ; fi
                  
                  
┌────────────────┐
│ Debugging Info │
└────────────────┘
                  
  SCAN ENVIRONMENT
  versions    - semgrep 1.103.0 on python 3.13.1                       
  environment - running in environment git, triggering event is unknown
                                                                                                                        
  Scanning 1263 files (only git-tracked) with 1057 Code rules:
            
  CODE RULES
                                                                                                                        
  Language      Rules   Files          Origin      Rules                                                                
 ─────────────────────────────        ───────────────────                                                               
  <multilang>      47    1144          Community    1057                                                                
  yaml             31     980                                                                                           
  go               83     145                                                                                           
  bash              4       5                                                                                           
                                                                                                                        
                    
  SUPPLY CHAIN RULES
                  
  No rules to run.
                  
          
  PROGRESS
   
  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00                                                                                                                        
                
                
┌──────────────┐
│ Scan Summary │
└──────────────┘
Some files were skipped or only partially analyzed.
  Scan was limited to files tracked by git.
  Partially scanned: 1 files only partially analyzed due to parsing or internal Semgrep errors
  Scan skipped: 1 files larger than 1.0 MB, 118 files matching .semgrepignore patterns
  For a full list of skipped files, run semgrep with the --verbose flag.

(need more rules? `semgrep login` for additional free Semgrep Registry rules)

CI scan completed successfully.
  Found 0 findings (0 blocking) from 1057 rules.
  No blocking findings so exiting with code 0

@kpfleming kpfleming changed the title Refactor: Comparison with errors using equality operators fails on wrapped errors refactor: Use errors.Is() instead of direct comparison. Jan 22, 2025
@kpfleming
Copy link
Contributor

This looks fine, but I'm curious whether there are any static analyzers we could add to the build process to report on this. If not, future contributions are likely to regress to using direct comparison.

@SahibYar
Copy link
Contributor Author

Thank you for reviewing my PR! You raised a good point about preventing future regressions. Static analyzers like golangci-lint can help enforce best practices for error handling. Specifically, the errcmp linter checks for improper direct comparisons of errors, encouraging the use of errors.Is() and errors.As() instead.

If you’re open to it, I could update the build process to include golangci-lint with the errorlint checker enabled, ensuring consistency in error handling across the codebase. Let me know if this sounds good, and I’d be happy to help set it up in a new PR both for go-fastly and cli.

@kpfleming
Copy link
Contributor

If you're willing to do that, that would be fine! Thanks again.

@kpfleming kpfleming merged commit dadeeb5 into fastly:main Jan 24, 2025
3 checks passed
@SahibYar SahibYar deleted the refactor/useErrorIs branch January 28, 2025 17:55
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.

2 participants