2010 | Use a Library for Reading Configuration from Environment Variables#2019
Open
nandhu-kumar wants to merge 1 commit into
Open
2010 | Use a Library for Reading Configuration from Environment Variables#2019nandhu-kumar wants to merge 1 commit into
nandhu-kumar wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR refactors the ChangesEnvconfig-driven configuration loading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/internal/config/engine.go`:
- Around line 87-94: The condition checking gatePort validity at the beginning
of the fallback block only validates against zero but allows negative values to
pass through as invalid port numbers. Modify the condition `if gatePort == 0` to
`if gatePort <= 0` to ensure that both zero and negative port values trigger the
fallback logic that defaults to either the parsed s.Port value or
defaultGatePort, preventing invalid negative port configurations from
propagating at runtime.
In `@esignet-service/internal/config/sunbird.go`:
- Around line 90-93: The EntityURL field in the SunbirdAuthn struct
initialization is missing whitespace trimming before slash normalization. Apply
strings.TrimSpace to the EntityURL value (similar to how SearchURL is handled)
before passing it to the trimTrailingSlash function to ensure leading and
trailing whitespace is removed before the trailing slash is normalized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a97d025-bbd6-4e7f-bafb-4b96bbcc2f44
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
esignet-service/.env.exampleesignet-service/README.mdesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/config/authn.goesignet-service/internal/config/clientmgmt.goesignet-service/internal/config/clientmgmt_test.goesignet-service/internal/config/db.goesignet-service/internal/config/engine.goesignet-service/internal/config/init.goesignet-service/internal/config/mosip.goesignet-service/internal/config/redis.goesignet-service/internal/config/redis_test.goesignet-service/internal/config/sunbird.goesignet-service/internal/config/types.goesignet-service/internal/config/types_test.go
nandhu-kumar
force-pushed
the
develop-2010
branch
from
eb04ef6 to
38e8257
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/internal/config/mosip.go`:
- Around line 40-54: The endpoint construction logic for certURL, sendOTP,
kycAuth, and kycExchange currently builds invalid relative paths when APIBaseURL
(apiBase) is empty. Guard all the fallback URL construction assignments (the
concatenation operations that append paths like "/mosip-certs/ida-partner.cer",
"/idauthentication/v1/otp/", etc.) by first checking if apiBase is non-empty
before executing these assignments. If apiBase is empty, these derived URLs
should not be constructed from the empty base.
In `@esignet-service/internal/config/redis.go`:
- Around line 112-113: The ConnMaxLifetime field currently allows negative
values to be used directly, which breaks the intended non-positive fallback
strategy. In the line where lifetime is assigned from s.ConnMaxLifetime, add a
check to clamp any negative ConnMaxLifetime value to 0 before creating the
time.Duration. This ensures that negative values are treated the same as 0
(meaning no limit), maintaining consistency with the other duration settings'
fallback behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cc054fe-ac92-4ca6-a91b-a06d0799eb2b
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
esignet-service/.env.exampleesignet-service/README.mdesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/config/authn.goesignet-service/internal/config/clientmgmt.goesignet-service/internal/config/clientmgmt_test.goesignet-service/internal/config/db.goesignet-service/internal/config/engine.goesignet-service/internal/config/init.goesignet-service/internal/config/mosip.goesignet-service/internal/config/redis.goesignet-service/internal/config/redis_test.goesignet-service/internal/config/sunbird.goesignet-service/internal/config/types.goesignet-service/internal/config/types_test.go
nandhu-kumar
force-pushed
the
develop-2010
branch
from
38e8257 to
e384671
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/internal/config/mosip.go`:
- Around line 37-57: The raw environment variable strings assigned to apiBase at
line 37 (via s.APIBaseURL) and domainURI at line 55 (via s.DomainURI) can
contain leading or trailing spaces, which bypass the empty string checks and
result in malformed URLs. Normalize whitespace by trimming leading and trailing
spaces from all raw env string assignments: s.APIBaseURL before passing to
trimTrailingSlash, s.DomainURI, s.SendOTPBaseURL, s.KYCAuthBaseURL, and
s.KYCExchangeBaseURL. Apply a whitespace trim function (such as
strings.TrimSpace) to each of these values before they are used in the empty
checks and URL construction logic.
In `@esignet-service/internal/config/sunbird.go`:
- Around line 73-88: The Sunbird field validation logic (for idField,
entityIDField, fieldDetails, and claimsMapping) currently only checks for empty
strings but does not handle whitespace-only values. Apply the strings.TrimSpace
function to each field before checking if it equals an empty string, so that
whitespace-only environment values are properly treated as empty and replaced
with their respective default values (defaultSunbirdIDField,
defaultSunbirdEntityIDField, defaultSunbirdFieldDetails,
defaultSunbirdClaimsMapping).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4730db4-7e48-4da4-9e7e-c44b37b4e509
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
esignet-service/.env.exampleesignet-service/README.mdesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/config/authn.goesignet-service/internal/config/clientmgmt.goesignet-service/internal/config/clientmgmt_test.goesignet-service/internal/config/db.goesignet-service/internal/config/engine.goesignet-service/internal/config/init.goesignet-service/internal/config/mosip.goesignet-service/internal/config/redis.goesignet-service/internal/config/redis_test.goesignet-service/internal/config/sunbird.goesignet-service/internal/config/types.goesignet-service/internal/config/types_test.go
nandhu-kumar
force-pushed
the
develop-2010
branch
from
e384671 to
230e995
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/internal/config/mosip.go`:
- Around line 47-55: The `s.LicenseKey` value is being used without trimming
whitespace in lines 47, 51, and 55 when constructing sendOTP, kycAuth, and
kycExchange URLs, and also at line 63 (as mentioned in the review), which
results in malformed endpoints when the license key has leading or trailing
whitespace. Normalize the license key once at the beginning by applying
strings.TrimSpace to s.LicenseKey and storing it in a variable (similar to how
apiBase, kycAuth, and kycExchange are handled), then use this trimmed variable
consistently throughout all URL derivations and for the MosipAuthn.LicenseKey
assignment to ensure clean endpoint construction.
In `@esignet-service/internal/config/redis_test.go`:
- Around line 95-103: The test TestLoadRedis_invalidNumbersFallBackToDefaults
currently validates invalid numeric fallbacks for REDIS_POOL_SIZE and
REDIS_DIAL_TIMEOUT_SECS but does not cover the negative value clamping behavior
for REDIS_CONN_MAX_LIFETIME_SECS. Add a test case by setting the environment
variable REDIS_CONN_MAX_LIFETIME_SECS to a negative value (such as -1) and then
add a require.Equal assertion to verify that the resulting cfg.ConnMaxLifetime
is clamped to 0 (representing no limit). This will lock in the regression test
for the negative-lifetime clamp path in the LoadRedis function.
In `@esignet-service/internal/config/types_test.go`:
- Around line 96-97: The test is unsetting the TEST_INIT_UNSET environment
variable but not preserving its pre-test state, which can cause test pollution.
Before calling os.Unsetenv on TEST_INIT_UNSET at line 96, capture its current
value using os.LookupEnv to determine whether it was originally set and what its
value was. Then in the t.Cleanup function, restore the original state by setting
the variable back to its original value if it was set, or leaving it unset if it
wasn't originally present. This ensures the test doesn't leak state to other
tests running in the same process.
In `@esignet-service/internal/config/types.go`:
- Around line 118-145: The process function calls applyStringDefaults even when
envconfig.Process returns an error, and the applyStringDefaults function uses
reflection on spec without validating it first. The reflect.ValueOf(spec).Elem()
call will panic if spec is nil, not a pointer, or not a struct. Add a return
statement in the process function after the envconfig.Process error check to
exit early, preventing applyStringDefaults from executing when configuration
processing fails, or add validation in applyStringDefaults to ensure spec is a
valid pointer to a struct before attempting reflection operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2b16eee-6ddc-4d2d-9884-a7b437454096
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
esignet-service/.env.exampleesignet-service/README.mdesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/config/authn.goesignet-service/internal/config/clientmgmt.goesignet-service/internal/config/clientmgmt_test.goesignet-service/internal/config/db.goesignet-service/internal/config/engine.goesignet-service/internal/config/init.goesignet-service/internal/config/mosip.goesignet-service/internal/config/mosip_test.goesignet-service/internal/config/redis.goesignet-service/internal/config/redis_test.goesignet-service/internal/config/sunbird.goesignet-service/internal/config/types.goesignet-service/internal/config/types_test.go
nandhu-kumar
force-pushed
the
develop-2010
branch
from
230e995 to
00ceb9c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
esignet-service/internal/config/types.go (1)
118-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
specshape before reflection to avoid startup panic paths.Line 122 can still reach
applyStringDefaults(spec)after a failedenvconfig.Process, and Line 133 dereferencesspecvia.Elem()without validating kind/nil. A nil/non-pointer/non-structspecwill panic.Suggested minimal fix
func process(spec any) { + rv := reflect.ValueOf(spec) + if !rv.IsValid() || rv.Kind() != reflect.Ptr || rv.IsNil() || rv.Elem().Kind() != reflect.Struct { + applog.GetLogger().Warn("process configuration", + applog.String("reason", "spec must be a non-nil pointer to struct")) + return + } if err := envconfig.Process("", spec); err != nil { applog.GetLogger().Warn("process configuration", applog.Error(err)) + return } applyStringDefaults(spec) }Also applies to: 132-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@esignet-service/internal/config/types.go` around lines 118 - 123, The process function calls applyStringDefaults(spec) unconditionally even after envconfig.Process fails, and applyStringDefaults dereferences spec via .Elem() without validating that spec is a non-nil pointer to a struct. Add a guard before calling applyStringDefaults to validate that spec is a non-nil pointer and has a valid struct kind, or alternatively add validation inside applyStringDefaults itself to check the type and nil status of spec before attempting to dereference it with .Elem().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@esignet-service/internal/config/types.go`:
- Around line 118-123: The process function calls applyStringDefaults(spec)
unconditionally even after envconfig.Process fails, and applyStringDefaults
dereferences spec via .Elem() without validating that spec is a non-nil pointer
to a struct. Add a guard before calling applyStringDefaults to validate that
spec is a non-nil pointer and has a valid struct kind, or alternatively add
validation inside applyStringDefaults itself to check the type and nil status of
spec before attempting to dereference it with .Elem().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2135c538-a920-40b9-9bbd-2c7da91bb186
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
esignet-service/.env.exampleesignet-service/README.mdesignet-service/cmd/esignet/main.goesignet-service/go.modesignet-service/internal/config/authn.goesignet-service/internal/config/clientmgmt.goesignet-service/internal/config/clientmgmt_test.goesignet-service/internal/config/db.goesignet-service/internal/config/engine.goesignet-service/internal/config/init.goesignet-service/internal/config/mosip.goesignet-service/internal/config/mosip_test.goesignet-service/internal/config/redis.goesignet-service/internal/config/redis_test.goesignet-service/internal/config/sunbird.goesignet-service/internal/config/types.goesignet-service/internal/config/types_test.go
nandhu-kumar
force-pushed
the
develop-2010
branch
2 times, most recently
from
8cab989 to
0c7a7d8
Compare
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
sacrana0
reviewed
Jun 26, 2026
nandhu-kumar
force-pushed
the
develop-2010
branch
from
0c7a7d8 to
9896715
Compare
sacrana0
reviewed
Jun 29, 2026
sacrana0
reviewed
Jun 29, 2026
nandhu-kumar
force-pushed
the
develop-2010
branch
from
9896715 to
2a52f42
Compare
sacrana0
reviewed
Jun 29, 2026
sacrana0
reviewed
Jun 29, 2026
sacrana0
reviewed
Jun 29, 2026
sacrana0
reviewed
Jun 29, 2026
sacrana0
reviewed
Jun 29, 2026
nandhu-kumar
force-pushed
the
develop-2010
branch
from
2a52f42 to
bc57637
Compare
sacrana0
reviewed
Jun 30, 2026
sacrana0
reviewed
Jun 30, 2026
sacrana0
reviewed
Jun 30, 2026
sacrana0
reviewed
Jun 30, 2026
sacrana0
reviewed
Jun 30, 2026
| // endpoint URLs are derived from MOSIP_API_INTERNAL_HOST and MOSIP_ESIGNET_MISP_KEY | ||
| // when not supplied individually, so they carry no envconfig default. | ||
| type mosipSpec struct { | ||
| LicenseKey string `envconfig:"MOSIP_ESIGNET_MISP_KEY"` |
Collaborator
There was a problem hiding this comment.
I believe this config is required? Mark all required configuration as 'required: true'. Check other places as well. And so no need of nil check in code.
Contributor
Author
There was a problem hiding this comment.
Thanks! I had one clarification regarding MOSIP_ESIGNET_MISP_KEY and MOSIP_API_INTERNAL_HOST before marking them as required:"true".
These two values are currently used only to derive the default IDA endpoint URLs when the individual IDA_URL values are not provided. If all the endpoint URLs are configured explicitly, these two values are never used by the runtime.
Should I still mark them as required:"true" and require them in all deployments, or do we want to continue supporting the configuration where all IDA endpoint URLs are provided explicitly without MOSIP_API_INTERNAL_HOST and MOSIP_ESIGNET_MISP_KEY?
MOSIP_P12_PATH and MOSIP_P12_PASSWORD are unconditionally required, so I'll keep those as required:"true"
sacrana0
reviewed
Jun 30, 2026
nandhu-kumar
force-pushed
the
develop-2010
branch
from
bc57637 to
bd0584e
Compare
…bles Signed-off-by: Nandhukumar <nandhukumare@gmail.com>
nandhu-kumar
force-pushed
the
develop-2010
branch
from
bd0584e to
3792c9d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
.envfrom the working directory at startup early enough to affect configuration and logging level.Documentation
.envexample to reflect.envloading behavior across development runs, builds, Docker, and tests, and that real environment variables override.env.Improvements
Tests
.envinitialization and configuration loaders (including Redis parsing and lenient “lax” decoding).