Add environment parameter in config

[mtwstudios]

Description

Checking the current environment / flagging by environment is a common use cases for projects I'm involved in, so this adds environment awareness to feature flags. You can set environment as a field in the Config passed to Init, then check them in rule (e.g. rule: env == "dev")

I realize I need to address tests/docs, but wanted to get feedback on the approach first to see if it was acceptable.

Changes include

• Bugfix (non-breaking change that solves an issue)
• New feature (non-breaking change that adds functionality)
• Breaking changes (change that is not backward-compatible and/or changes current functionality)

Closes issue(s)

Resolve #175

Checklist

• I have tested this code
• I have added unit test to cover this code
• I have updated the documentation (README.md and /docs)
• I have followed the contributing guide

[coveralls]

Coverage increased (+0.02%) to 93.975% when pulling a2f1c0f on neonmoose:main into 45913df on thomaspoignant:main.

[thomaspoignant]

Hey, @mtwstudios thanks for your contribution this is awesome.
The approach you have is the one I had in mind when writing the issue so it looks good to me.

Despite that, I made some minor change requests in your pull request to avoid changing unused things.

I would also suggest adding some tests where you are testing the environment feature because in your test you are always sending an empty environment so it will not test that this is working as expected.

And as you said, it could be great if you can add some associated documentation.

[mtwstudios]

Thanks for the feedback @thomaspoignant ! I think I've addressed everything, including the cleanup, and adding tests and docs for the change. Let me know if there's any other changes you think are needed.

[thomaspoignant]

Hey @mtwstudios sorry for the delay in the review.

I was testing the solution and tell me if I am wrong but from what I understand with this solution if we configure an env, will have to create a rule to catch the environment.

Something like this:

new-admin-access:
  percentage: 100
  true: true
  false: false
  default: false
  rule: env eq "pre"

I was wondering if we could not have something easier.
Something like this:

new-admin-access:
  percentage: 100
  true: true
  false: false
  default: false
  environment: pre

The goal for me is to be able to dedicate a flag to a specific environment easily.
What do you think about it?

For me the rule should be, that if we have an environment in the config of the flag this is available only for this environment if we remove it, it should be available for all the environments.

My only concern is that we cannot have 2 different configurations for the same flag, and it could be a limitation.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[mtwstudios]

Hey @thomaspoignant, yep - with this solution, env would be specified in the rule.

The advantage of having it be specified in the rule is that it allows for more flexibility for various use cases. In particular:

• Enabling for multiple (but not all) environments. This is a very common use case on teams I've been on - for example to enable a flag in local development environments and staging environments but not in production
  • e.g. rule: (env == "dev") or (env == "staging")
• Enabling the flag based on certain rules only in certain environments. For example it's enabled for all users in non-production environments, but in production it's only enabled for specific users
  • e.g. rule: (env != "prod") or (user_id == 1234)

I think having environment separate from rule, while might be easier in the simplest case, prevents it from being used in cases like the examples above. This would make it much less useful/powerful in my opinion.

My only concern is that we cannot have 2 different configurations for the same flag, and it could be a limitation.

I'm not sure what you mean by this - can you give an example?

[thomaspoignant]

You are right about the usage this is way more extensible to have it in the rules.
I will do a follow-up PR to add a section in the documentation to make it extra clear on how to deal with the environment.

I will release a new version today containing this new version.