Skip to content
This repository was archived by the owner on Oct 10, 2025. It is now read-only.

chore: update from urface/cli/v2 to v3 and fix a few outdated files - #672

Merged
dbarrosop merged 7 commits into
mainfrom
urfave-v3
Sep 1, 2025
Merged

chore: update from urface/cli/v2 to v3 and fix a few outdated files#672
dbarrosop merged 7 commits into
mainfrom
urfave-v3

Conversation

@dbarrosop

@dbarrosop dbarrosop commented Aug 30, 2025

Copy link
Copy Markdown
Member

PR Type

Enhancement


Description

  • Migrate from urfave/cli/v2 to v3 with API changes

  • Update CLI flag configuration to use Sources instead of EnvVars

  • Refactor function signatures to use cli.Command instead of cli.Context

  • Add CLI documentation generation command


Diagram Walkthrough

flowchart LR
  A["urfave/cli/v2"] --> B["urfave/cli/v3"]
  B --> C["Updated Flag Configuration"]
  B --> D["Function Signature Changes"]
  B --> E["CLI Documentation Generation"]
  C --> F["EnvVars → Sources"]
  D --> G["cli.Context → cli.Command"]
Loading

File Walkthrough

Relevant files
Enhancement
10 files
config.go
Update config functions for cli/v3 compatibility                 
+46/-46 
db.go
Refactor database pool creation for new CLI                           
+5/-4     
email.go
Update email configuration and error handling                       
+27/-27 
enum.go
Simplify enum value handling and remove helpers                   
+8/-29   
jwt_getter.go
Update JWT getter configuration for cli/v3                             
+11/-11 
logger.go
Refactor logging configuration and flag processing             
+20/-12 
migrations.go
Update migration functions for new CLI interface                 
+10/-10 
oauth.go
Update OAuth provider configuration functions                       
+96/-94 
serve.go
Major CLI flag migration and server configuration               
+218/-218
main.go
Add CLI documentation generation and update app structure
+57/-9   
Miscellaneous
5 files
add_security_key_test.go
Add nolintlint directive to test constant                               
+1/-1     
elevate_webauthn_test.go
Add nolintlint directive to test constant                               
+1/-1     
sign_in_webauthn_test.go
Add nolintlint directive to test constant                               
+1/-1     
index.html
Remove WebAuthn test HTML file                                                     
+0/-194 
audit-ci.jsonc
Remove Node.js audit configuration file                                   
+0/-7     
Documentation
7 files
CLAUDE.md
Update development guide title and content                             
+51/-42 
DEVELOPERS.md
Remove outdated Node.js development guide                               
+0/-77   
README.md
Update installation and configuration documentation           
+11/-9   
SECURITY.md
Update security vulnerability reporting URL                           
+1/-1     
cli.md
Add comprehensive CLI documentation                                           
+481/-0 
configuration.md
Update configuration documentation references                       
+1/-1     
environment-variables.md
Remove outdated environment variables documentation           
+0/-127 
Dependencies
2 files
go.mod
Update dependencies for urfave/cli/v3 migration                   
+7/-4     
go.sum
Update dependency checksums for new versions                         
+6/-8     

@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Error Handling

The GravatarDefault field assignment on line 99 directly calls cmd.String() without using the GetEnumValue helper that was removed, potentially causing runtime issues if the enum validation is not properly handled elsewhere.

GravatarRating:              cmd.String(flagGravatarRating),
Breaking Change

The removal of GetEnumValue and GetGeneric helper functions may break existing code that depends on these utilities for type-safe enum value retrieval from CLI contexts.

	if s, ok := e.Get().(string); ok {
		return s
	}

	return ""
}
API Inconsistency

The migration changes cli.NewStringSlice() calls to direct slice literals, but this may not be equivalent behavior in urfave/cli v3. The new Value field assignments should be verified to work correctly with the v3 API.

	Sources:  cli.EnvVars("AUTH_USER_DEFAULT_ALLOWED_ROLES"),
},
&cli.StringFlag{ //nolint: exhaustruct
	Name:     flagDefaultRole,
	Usage:    "Default user role for registered users",
	Category: "signup",
	Value:    "user",
	Sources:  cli.EnvVars("AUTH_USER_DEFAULT_ROLE"),
},
&cli.StringFlag{ //nolint: exhaustruct
	Name:     flagDefaultLocale,
	Usage:    "Default locale",
	Category: "signup",
	Value:    "en",
	Sources:  cli.EnvVars("AUTH_LOCALE_DEFAULT"),
},
&cli.StringSliceFlag{ //nolint: exhaustruct
	Name:     flagAllowedLocales,
	Usage:    "Allowed locales",
	Category: "signup",
	Value:    []string{"en"},
	Sources:  cli.EnvVars("AUTH_LOCALE_ALLOWED_LOCALES"),

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Maintain string return type safety

The return type changed from string to any which breaks type safety. This could
cause runtime panics when callers expect a string but receive a different type.

go/cmd/enum.go [17-22]

-func (e *EnumValue) Get() any {
+func (e *EnumValue) Get() string {
     if e.selected == "" {
         return e.Default
     }
     
     return e.selected
 }
Suggestion importance[1-10]: 8

__

Why: This is a valid concern about type safety. Changing the return type from string to any breaks the contract and could cause runtime issues for existing callers expecting a string type, especially in the String() method implementation.

Medium
Security
Avoid exposing sensitive configuration values

The error message reveals the invalid auth method value to potential attackers.
Consider using a generic error message that doesn't expose the actual input value to
prevent information leakage.

go/cmd/email.go [35-43]

 switch cmd.String(flagSMTPAuthMethod) {
 case "LOGIN":
     auth = notifications.LoginAuth(user, password, host)
 case "PLAIN":
     auth = notifications.PlainAuth("", user, password, host)
 case "CRAM-MD5":
     auth = smtp.CRAMMD5Auth(user, password)
 default:
-    return nil, fmt.Errorf("unknown SMTP auth method (%s)", cmd.String(flagSMTPAuthMethod))
+    return nil, errors.New("unsupported SMTP auth method")
 }
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential information leakage issue where invalid auth method values are exposed in error messages. However, the impact is relatively low since this is configuration validation rather than user input, and the suggested generic error message reduces debugging capability.

Low

@dbarrosop
dbarrosop merged commit ccbc12f into main Sep 1, 2025
8 checks passed
@dbarrosop
dbarrosop deleted the urfave-v3 branch September 1, 2025 10:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants