Add warn for helper func#38
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces structured logging to environment variable configuration helpers using the slog package. The review feedback highlights that logging warnings for missing environment variables in 'IfPresent' functions is inappropriate for optional configurations and will cause excessive log noise. Furthermore, logging the raw values of environment variables when parsing fails is flagged as a security risk due to potential sensitive information leakage; the reviewer recommends logging the error object instead.
| slog.Warn("invalid_bool", "env", key, "value", v) | ||
| } | ||
| } else { | ||
| slog.Warn("missing_env", "env", key) | ||
| } |
There was a problem hiding this comment.
This change introduces two issues:
- Security: Logging the raw value
vfrom an environment variable can leak sensitive information if a secret was accidentally assigned to the wrong key. It is safer to log the error returned by the parser. - Log Noise: Warning when an optional environment variable is missing is misleading for "IfPresent" helpers.
Consider removing the missing environment warning and logging the error instead of the raw value.
slog.Warn("invalid_bool", "env", key, "err", err)
}
}| slog.Warn("invalid_int", "env", key, "value", v) | ||
| } | ||
| } else { | ||
| slog.Warn("missing_env", "env", key) | ||
| } |
There was a problem hiding this comment.
Similar to setBoolIfPresent, logging the raw value v poses a security risk of leaking secrets into logs. Additionally, warning on missing optional variables creates unnecessary log noise. It is recommended to log the parsing error and remove the warning for missing variables.
slog.Warn("invalid_int", "env", key, "err", err)
}
}| } else { | ||
| slog.Warn("missing_env", "env", key) | ||
| } |
There was a problem hiding this comment.
The setStringIfPresent function is designed for optional environment variables. Logging a warning when the variable is missing contradicts the "IfPresent" semantics and will result in excessive log noise for optional configuration. If a variable is mandatory, it should be handled by a separate validation step or a different helper function.
}1 participant
No description provided.