Fix 1 DAGSTER_AUTH_ADMIN_PASSWORD bootstrap bug and 1 logging bug for DAGSTER_AUTH_SECRET_KEY#2
Fix 1 DAGSTER_AUTH_ADMIN_PASSWORD bootstrap bug and 1 logging bug for DAGSTER_AUTH_SECRET_KEY#2hongha912 wants to merge 1 commit into
Conversation
_bootstrap_admin does self.config.get("DAGSTER_AUTH_ADMIN_PASSWORD"), but self.config is config.__dict__ from the AuthConfig singleton. The AuthConfig stores it as self.ADMIN_PASSWORD, not self.DAGSTER_AUTH_ADMIN_PASSWORD:
self.ADMIN_PASSWORD = os.getenv("DAGSTER_AUTH_ADMIN_PASSWORD", "")
So self.config.get("DAGSTER_AUTH_ADMIN_PASSWORD") returns None — it should be self.config.get("ADMIN_PASSWORD").
Same issue with SECRET_KEY — the config stores it as self.SECRET_KEY but the warning prints because _generate_secret_key() is called as the default before the env var is checked... wait no, looking again:
self.SECRET_KEY = os.getenv("DAGSTER_AUTH_SECRET_KEY", self._generate_secret_key())
Python evaluates self._generate_secret_key() even when the env var exists, because default arguments are eagerly evaluated. That's the warning source — it's a bug in dagster-authkit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTwo configuration handling changes in the authentication framework: the admin password bootstrap now reads from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Hi, thank you for starting this project. I was testing for my OSS deployment on GCP Cloud Run, which has limitation around SSH-ing into the deployment environment. When trying to create an admin user via the boostrapping process, I came across 2 bugs:
Bug 1
_bootstrap_admindoesself.config.get("DAGSTER_AUTH_ADMIN_PASSWORD"), butself.configisconfig.__dict__from theAuthConfigsingleton. The AuthConfig stores it asself.ADMIN_PASSWORD, notself.DAGSTER_AUTH_ADMIN_PASSWORDBug 2: More of a logging bug:
self.SECRET_KEY = os.getenv("DAGSTER_AUTH_SECRET_KEY", self._generate_secret_key()): Python will evaluate self._generate_secret_key() even when the env var exists, because default arguments are eagerly evaluated. The default creates a warning log which caused things to go astray.Summary by CodeRabbit
ADMIN_PASSWORDenvironment key