Skip to content

CLOUDP-239791: Replace log.Fatal calls in main.go with run() error pattern #941

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.

Sign up for GitHub

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

Open
m1kola wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CLOUDP-239791: Replace log.Fatal calls in main.go with run() error pattern #941

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ func (c *crdsToWatch) String() string {
}

func main() {
if err := run(); err != nil {
log.Error(err)
os.Exit(1)
Comment on lines +114 to +115
Copy link
Copy Markdown
Collaborator

@josvazg josvazg Mar 27, 2026

Choose a reason for hiding this comment

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

q: why not making this a fatal? I would also wrap the error with some info.

Suggested change
log.Error(err)
os.Exit(1)
log.Fatalf("Fatal error: %v", err)

Copy link
Copy Markdown
Contributor Author

@m1kola m1kola Mar 27, 2026

Choose a reason for hiding this comment

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

I normally prefer explicit over implicit. And log.Fatalf has an implicit exit.

But I don't mind changing it to log.Fatalf, if you prefer.

}
}

func run() error {
flag.Parse()
// If no CRDs are specified, we set default to non-multicluster CRDs
if len(crds) == 0 {
Expand Down Expand Up @@ -199,13 +206,13 @@ func main() {

mgr, err := ctrl.NewManager(cfg, managerOptions)
if err != nil {
log.Fatal(err)
return err
Copy link
Copy Markdown
Collaborator

@josvazg josvazg Mar 27, 2026

Choose a reason for hiding this comment

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

nit: it is usually best to wrap the error context. E.g:

Suggested change
return err
return fmt.Errorf("failed to create manager: %w", err)

Copy link
Copy Markdown
Contributor Author

@m1kola m1kola Mar 27, 2026

Choose a reason for hiding this comment

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

This is startup code in main, not a library/package. The errors from ctrl.NewManager, apiv1.AddToScheme, etc. already return descriptive messages. Wrapping them adds boilerplate without giving the caller anything useful. And there's no caller that needs to errors.Is/errors.As these, and the log output in main will print the original message anyway.

The point of the ticket was to fix the os.Exit bypass, not to improve error messages.

}
log.Info("Registering Components.")

// Setup Scheme for all resources
if err := apiv1.AddToScheme(scheme); err != nil {
log.Fatal(err)
return err
}

// memberClusterObjectsMap is a map of clusterName -> clusterObject
Expand All @@ -214,7 +221,7 @@ func main() {
if slices.Contains(crds, mongoDBMultiClusterCRDPlural) {
memberClustersNames, err := getMemberClusters(ctx, cfg, currentNamespace)
if err != nil {
log.Fatal(err)
return err
}

log.Infof("Watching Member clusters: %s", memberClustersNames)
Expand All @@ -225,7 +232,7 @@ func main() {

memberClusterClients, err := multicluster.CreateMemberClusterClients(memberClustersNames, multicluster.GetKubeConfigPath())
if err != nil {
log.Fatal(err)
return err
}

// Add the cluster object to the manager corresponding to each member clusters.
Expand Down Expand Up @@ -253,35 +260,35 @@ func main() {
log.Infof("Adding cluster %s to cluster map.", k)
memberClusterObjectsMap[k] = cluster
if err = mgr.Add(cluster); err != nil {
log.Fatal(err)
return err
}
}
}

// Setup all Controllers
if slices.Contains(crds, mongoDBCRDPlural) {
if err := setupMongoDBCRD(ctx, mgr, imageUrls, initDatabaseNonStaticImageVersion, databaseNonStaticImageVersion, forceEnterprise, enableClusterMongoDBRoles, agentDebug, agentDebugImage, memberClusterObjectsMap); err != nil {
log.Fatal(err)
return err
}
}
if slices.Contains(crds, mongoDBOpsManagerCRDPlural) {
if err := setupMongoDBOpsManagerCRD(ctx, mgr, memberClusterObjectsMap, imageUrls, initDatabaseNonStaticImageVersion, initOpsManagerImageVersion); err != nil {
log.Fatal(err)
return err
}
}
if slices.Contains(crds, mongoDBUserCRDPlural) {
if err := setupMongoDBUserCRD(ctx, mgr, memberClusterObjectsMap); err != nil {
log.Fatal(err)
return err
}
}
if slices.Contains(crds, mongoDBMultiClusterCRDPlural) {
if err := setupMongoDBMultiClusterCRD(ctx, mgr, imageUrls, initDatabaseNonStaticImageVersion, databaseNonStaticImageVersion, forceEnterprise, enableClusterMongoDBRoles, agentDebug, agentDebugImage, memberClusterObjectsMap); err != nil {
log.Fatal(err)
return err
}
}
if slices.Contains(crds, mongoDBSearchCRDPlural) {
if err := setupMongoDBSearchCRD(ctx, mgr); err != nil {
log.Fatal(err)
return err
}
}

Expand All @@ -302,7 +309,7 @@ func main() {
env.ReadOrPanic(mcoConstruct.VersionUpgradeHookImageEnv),
env.ReadOrPanic(mcoConstruct.ReadinessProbeImageEnv),
); err != nil {
log.Fatal(err)
return err
}
}

Expand Down Expand Up @@ -331,9 +338,7 @@ func main() {

log.Info("Starting the Cmd.")

if err := mgr.Start(ctx); err != nil {
log.Fatal(err)
}
return mgr.Start(ctx)
}

func startRootSpan(currentNamespace string, spanIDHex string, traceCtx context.Context) (context.Context, trace.Span) {
Expand Down
Loading