Skip to content

Conversation

@minhd-vu
Copy link
Contributor

@minhd-vu minhd-vu commented Oct 3, 2025

Description

  • Remove flag_loader package to the root directory
  • Combine some logic to improve flag parsing

Jira / Linear Tickets

Testing

  • Test A
  • Test B

@minhd-vu minhd-vu marked this pull request as draft October 3, 2025 17:11
@minhd-vu minhd-vu force-pushed the minhd-vu/flag-loader branch from 5089e39 to 3c6a40d Compare October 3, 2025 17:45
@minhd-vu minhd-vu marked this pull request as ready for review October 3, 2025 19:28
leovct
leovct previously approved these changes Oct 6, 2025
@minhd-vu minhd-vu requested a review from leovct October 6, 2025 19:09
@minhd-vu minhd-vu changed the title chore: remove flag_loader chore: refactor flag_loader Oct 6, 2025
@minhd-vu minhd-vu marked this pull request as draft October 6, 2025 19:28
@minhd-vu minhd-vu marked this pull request as ready for review October 6, 2025 19:32
leovct
leovct previously approved these changes Oct 6, 2025
leovct
leovct previously approved these changes Oct 8, 2025
@minhd-vu
Copy link
Contributor Author

minhd-vu commented Oct 8, 2025

Cobra Best Practices Research

I've done some research on Cobra best practices to improve our CLI development. Here are the key findings:

Flag Sharing Patterns

  • Struct with AddFlags() method is the most common pattern in popular projects (kubectl, Docker CLI, Helm)
  • Allows sharing flags horizontally across unrelated commands
  • Better than PersistentFlags for non-parent-child relationships

Viper + Cobra Integration

  • Configuration precedence: Flags > Environment Variables > Config File > Defaults
  • Critical timing: Bind flags in PersistentPreRunE (after parsing, before execution), NOT in init()
  • Always read from Viper after binding, never from flag variables
  • Use mapstructure tags to map dash-containing flag names (e.g., --write-to-tx) to Go struct fields (e.g., WriteToTx)

Key Pattern for Viper

PersistentPreRunE: func(cmd *cobra.Command, args []string) error {
    return viper.BindPFlags(cmd.Flags())  // Magic happens here
}

Resources

  • Real-world examples from kubernetes/kubectl, docker/cli, helm/helm
  • Flag sharing: Struct + AddFlags() method pattern is industry standard
  • Viper unmarshalling: Use mapstructure tags for any flag name → struct field mapping

@minhd-vu minhd-vu marked this pull request as draft October 17, 2025 15:09
@minhd-vu minhd-vu closed this Jan 16, 2026
@minhd-vu minhd-vu reopened this Jan 16, 2026
@minhd-vu minhd-vu force-pushed the minhd-vu/flag-loader branch from f54a47e to 5256aa1 Compare January 16, 2026 16:20
@minhd-vu minhd-vu marked this pull request as ready for review January 16, 2026 16:36
Copy link
Contributor

@jhkimqd jhkimqd left a comment

Choose a reason for hiding this comment

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

+1 lgtm

Copy link
Contributor

@jhkimqd jhkimqd left a comment

Choose a reason for hiding this comment

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

Let's merge #822 into minhd-vu/flag-loader first then we could merge this into main - wdyt?

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