Skip to content

Commit

Permalink
fix parsing of grafana feature flags that're enabled via the feature_…
Browse files Browse the repository at this point in the history
…toggles.enabled syntax (#2477)

# What this PR does

Grafana provides two methods for enabling feature flags:
- `feature_toggles.enabled`
- `feature_toggles.<name_of_feature_flag>`

For example, to enable `accessControlOnCall` you could either do:
```json
{
  "feature_toggles": {
    "enabled": "accessControlOnCall,someOtherCoolFeatureFlag"
  }
}
```

or:
```json
{
  "feature_toggles": {
    "accessControlOnCall": true,
    "someOtherCoolFeatureFlag": true
  }
}
```

In method 1, if they're multiple feature flags present, they are _comma
separated, not space separated_. This PR fixes this parsing issue.

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required) (N/A)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
  • Loading branch information
joeyorlando authored Jul 10, 2023
1 parent f84587f commit 182f18d
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 8 deletions.
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Modified DRF pagination class used by `GET /api/internal/v1/alert_receive_channels` and `GET /api/internal/v1/schedules`
endpoints so that the `next` and `previous` pagination links are properly set when OnCall is run behind
a reverse proxy by @joeyorlando ([#TBD](https://github.com/grafana/oncall/pull/TBD))
a reverse proxy by @joeyorlando ([#2467](https://github.com/grafana/oncall/pull/2467))

### Fixed

- Address issue where we were improperly parsing Grafana feature flags that were enabled via the `feature_flags.enabled`
method by @joeyorlando ([#2477](https://github.com/grafana/oncall/pull/2477))

## v1.3.7 (2023-07-06)

Expand Down
4 changes: 2 additions & 2 deletions engine/apps/grafana_plugin/helpers/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,8 @@ def _feature_toggle_is_enabled(self, instance_info: GCOMInstanceInfo, feature_na
if not instance_feature_toggles:
return False

# features enabled via enable key are space separated
features_enabled_via_enable_key = instance_feature_toggles.get("enable", "").split(" ")
# features enabled via enable key are comma separated (https://github.com/grafana/grafana/issues/36511)
features_enabled_via_enable_key = instance_feature_toggles.get("enable", "").split(",")
feature_enabled_via_enable_key = feature_name in features_enabled_via_enable_key
feature_enabled_via_direct_key = instance_feature_toggles.get(feature_name, "false") == "true"

Expand Down
12 changes: 7 additions & 5 deletions engine/apps/grafana_plugin/tests/test_gcom_api_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,27 @@ def test_it_returns_based_on_feature_toggle_value(
({}, False),
({"config": {}}, False),
({"config": {"feature_toggles": {}}}, False),
({"config": {"feature_toggles": {"enable": "foo bar baz"}}}, False),
({"config": {"feature_toggles": {"enable": "foo,bar,baz"}}}, False),
({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "false"}}}, False),
# must be space separated
({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE}baz"}}}, False),
# must be comma separated
({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE}baz"}}}, False),
# these cases will probably never happen, but lets account for them anyways
(
{
"config": {
"feature_toggles": {
"enable": f"foo bar baz {TEST_FEATURE_TOGGLE}",
"enable": f"foo,bar,baz,{TEST_FEATURE_TOGGLE}",
TEST_FEATURE_TOGGLE: "false",
}
}
},
True,
),
({"config": {"feature_toggles": {"enable": f"foo bar baz", TEST_FEATURE_TOGGLE: "true"}}}, True),
({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, True),
({"config": {"feature_toggles": {TEST_FEATURE_TOGGLE: "true"}}}, True),
# features enabled via feature_toggles.enable should be comma separated, not space separated
({"config": {"feature_toggles": {"enable": f"foo,bar,{TEST_FEATURE_TOGGLE},baz"}}}, True),
({"config": {"feature_toggles": {"enable": f"foo bar {TEST_FEATURE_TOGGLE} baz"}}}, False),
],
)
def test_feature_toggle_is_enabled(self, instance_info, expected) -> None:
Expand Down

0 comments on commit 182f18d

Please sign in to comment.