refactor(parameters): replace EnvironmentVariablesService class with helper functions in Parameters

[JonkaSusaki]

Summary

Changes

This PR removes the legacy class-based EnvironmentVariablesService from the Parameters package and replaces it with functional local helpers.

• Removed the packages/parameters/src/config/EnvironmentVariablesService.ts file.
• Removed the packages/parameters/tests/unit/EnvironmentVariablesService.test.ts file, since our tests are backed by commons/tests/unit/envUtils.test.ts.
• Created the packages/parameters/src/utils/env.ts file containing the helpers.
• Changed the contents of packages/parameters/src/base/BaseProvider.ts.
• Changed the contents of packages/parameters/src/base/GetMultipleOptions.ts.
• Changed the contents of packages/parameters/src/base/GetOptions.ts.
• Changed the contents of packages/parameters/src/appconfig/AppConfigProvider.ts.
• Added the isValueTrue function to the packages/commons/src/envUtils.ts because it is needed in packages/parameters/src/ssm/SSMProvider.ts.
• Added the tests for isValueTrue in packages/commons/tests/unit/envUtils.test.ts.
• Changed the contents of packages/parameters/src/ssm/SSMProvider.ts.

Issue number: #4138

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[boring-cyborg]

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
22.4% Duplication on New Code

See analysis details on SonarQube Cloud

[svozza]

I think this is correct, we probably just want this to fail if the environment variable is not read properly.

[JonkaSusaki]

I think we need to catch it, since the getNumberFromEnv throws an error at packages/commons/src/envUtils.ts at line 110.

When removing the try/catch block from packages/parameters/src/utils/env.ts, the AppConfigProvider class can't instantiate the packages/parameters/src/base/GetOptions.ts in case the env does not exists.

Instead of setting the maxAge to DEFAULT_MAX_AGE_SECS, everything just breaks because of that thrown error.

What do you think? Perhaps I could add a console.warn with the error in the catch block to avoid this sonarqube error.

[svozza]

ts in case the env does not exists.

Ah yes, that's a good point. Another possibility is to return the default value as defined in the constants file:

import { DEFAULT_MAX_AGE_SECS } from '../constants.js';

// ...

  } catch (error) {
    return DEFAULT_MAX_AGE_SECS;
  }

I'd like to hear @dreamorosi thoughts first though.

[dreamorosi]

In this case, I think it'd be sufficient to do this:

return getNumberFromEnv({
  key: 'POWERTOOLS_PARAMETERS_MAX_AGE',
  // will return `undefined` and not throw if the env var is not set or empty
  defaultValue: undefined,
})

It's ok to return undefined here because in the place where we're calling it (here) we're already doing this:

this.maxAge = getParametersMaxAge() ?? DEFAULT_MAX_AGE_SECS;

Since this getParametersMaxAge is called only in this place, I'd even consider/suggest to call the getNumberFromEnv there directly, so we can save a file and reduce cognitive load.

[svozza]

Thanks for picking this up, it's looking good! I only had one small comment.

[dreamorosi]

In this case, I think it'd be sufficient to do this:

return getNumberFromEnv({
  key: 'POWERTOOLS_PARAMETERS_MAX_AGE',
  // will return `undefined` and not throw if the env var is not set or empty
  defaultValue: undefined,
})

It's ok to return undefined here because in the place where we're calling it (here) we're already doing this:

this.maxAge = getParametersMaxAge() ?? DEFAULT_MAX_AGE_SECS;

Since this getParametersMaxAge is called only in this place, I'd even consider/suggest to call the getNumberFromEnv there directly, so we can save a file and reduce cognitive load.

[dreamorosi]

I don't think we need to add this one, we can use the existing getBooleanFromEnv above with extendedParsing set to true

[dreamorosi]

I would suggest replacing this getStringFromEnv with a call to getBooleanFromEnv with extendedParsing set to true & defaultValue to false.

This would change the return value to a boolean and we could simplify further the call in the SSMProvider class, since this is used in a if/else statement.

Also like discussed in the other comment above, I'd consider/suggest moving the getBooleanFromEnv directly in the SSMProvider so we can remove this file entirely.