Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: default variants #3271

Merged
merged 32 commits into from
Jul 23, 2024
Merged

feat: default variants #3271

merged 32 commits into from
Jul 23, 2024

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Jul 12, 2024

Redo of #3168

Fixes: #3134

CleanShot 2024-07-19 at 13 37 59@2x

CleanShot 2024-07-19 at 13 50 07@2x

CleanShot 2024-07-19 at 13 51 47@2x
CleanShot 2024-07-19 at 13 51 58@2x

TODO (In this PR)

  • Add tests
  • Implement in UI
  • Add for other relational dbs
  • Import/Export support (will also influence declarative backends)
  • Add to snapshot builder code (declarative)

In other repos

Signed-off-by: Mark Phelps <[email protected]>
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 57.41935% with 66 lines in your changes missing coverage. Please review.

Project coverage is 64.18%. Comparing base (0b13a4e) to head (5c193ff).

Files Patch % Lines
internal/storage/sql/mysql/mysql.go 0.00% 23 Missing ⚠️
internal/server/evaluation/data/server.go 0.00% 17 Missing ⚠️
internal/storage/sql/postgres/postgres.go 0.00% 10 Missing ⚠️
internal/ext/importer.go 75.00% 3 Missing and 2 partials ⚠️
internal/storage/sql/common/flag.go 86.84% 2 Missing and 3 partials ⚠️
internal/storage/sql/sqlite/sqlite.go 60.00% 3 Missing and 1 partial ⚠️
internal/server/evaluation/evaluation.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3271      +/-   ##
==========================================
- Coverage   64.24%   64.18%   -0.06%     
==========================================
  Files         168      168              
  Lines       13410    13506      +96     
==========================================
+ Hits         8615     8669      +54     
- Misses       4137     4174      +37     
- Partials      658      663       +5     
Flag Coverage Δ
unittests 64.18% <57.41%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@markphelps markphelps marked this pull request as ready for review July 19, 2024 18:55
@markphelps markphelps requested a review from a team as a code owner July 19, 2024 18:55
@markphelps
Copy link
Collaborator Author

I'm opening this up for feedback before I get too far in the weeds with adding tests

cc @erka

@erka
Copy link
Collaborator

erka commented Jul 19, 2024

I'm opening this up for feedback before I get too far in the weeds with adding tests

cc @erka

@markphelps Overall, it looks fantastic. There is one white spot with evaluation.proto. I don't see that we set a default variant there.

Signed-off-by: Mark Phelps <[email protected]>
Signed-off-by: Mark Phelps <[email protected]>
@markphelps
Copy link
Collaborator Author

I'm opening this up for feedback before I get too far in the weeds with adding tests
cc @erka

@markphelps Overall, it looks fantastic. There is one white spot with evaluation.proto. I don't see that we set a default variant there.

Im not quite sure what you mean. Would you mind showing me in the a comment in the PR if possible?

@erka
Copy link
Collaborator

erka commented Jul 19, 2024

Im not quite sure what you mean. Would you mind showing me in the a comment in the PR if possible?

If I set the default variant for a flag and then look at snapshot at http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default I don't see any changes in the response.

@markphelps
Copy link
Collaborator Author

Im not quite sure what you mean. Would you mind showing me in the a comment in the PR if possible?

If I set the default variant for a flag and then look at snapshot at http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default I don't see any changes in the response.

ah yes! good catch. will add. and also need to add support in the client side evaluator engine

@markphelps
Copy link
Collaborator Author

Eval Data API

With Proper Header Set

curl -H "X-Flipt-Accept-Server-Version: 1.47.0" http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default | jq

{
  "namespace": {
    "key": "default"
  },
  "flags": [
    {
      "key": "fgsdaf",
      "name": "fgsdaf",
      "description": "",
      "enabled": false,
      "type": "BOOLEAN_FLAG_TYPE",
      "createdAt": "2024-07-19T14:18:19.835699Z",
      "updatedAt": "2024-07-19T14:18:19.835699Z",
      "rules": [],
      "rollouts": [
        {
          "type": "THRESHOLD_ROLLOUT_TYPE",
          "rank": 1,
          "threshold": {
            "percentage": 50,
            "value": true
          }
        }
      ]
    },
    {
      "key": "dfsdf",
      "name": "dfsdf",
      "description": "",
      "enabled": true,
      "type": "VARIANT_FLAG_TYPE",
      "createdAt": "2024-07-19T14:20:48.359953Z",
      "updatedAt": "2024-07-21T16:59:59.373240Z",
      "rules": [],
      "rollouts": [],
      "defaultVariant": {
        "id": "e468f221-7e48-4dcf-8f1f-3156a990f363",
        "key": "goo",
        "attachment": ""
      }
    }
  ]
}

With Lower Version Header Set

curl -H "X-Flipt-Accept-Server-Version: 1.46.0" http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default | jq

{
  "namespace": {
    "key": "default"
  },
  "flags": [
    {
      "key": "fgsdaf",
      "name": "fgsdaf",
      "description": "",
      "enabled": false,
      "type": "BOOLEAN_FLAG_TYPE",
      "createdAt": "2024-07-19T14:18:19.835699Z",
      "updatedAt": "2024-07-19T14:18:19.835699Z",
      "rules": [],
      "rollouts": [
        {
          "type": "THRESHOLD_ROLLOUT_TYPE",
          "rank": 1,
          "threshold": {
            "percentage": 50,
            "value": true
          }
        }
      ]
    },
    {
      "key": "dfsdf",
      "name": "dfsdf",
      "description": "",
      "enabled": true,
      "type": "VARIANT_FLAG_TYPE",
      "createdAt": "2024-07-19T14:20:48.359953Z",
      "updatedAt": "2024-07-21T16:59:59.373240Z",
      "rules": [],
      "rollouts": []
    }
  ]
}

With No Header Set

curl http://localhost:8080/internal/v1/evaluation/snapshot/namespace/default | jq

{
  "namespace": {
    "key": "default"
  },
  "flags": [
    {
      "key": "fgsdaf",
      "name": "fgsdaf",
      "description": "",
      "enabled": false,
      "type": "BOOLEAN_FLAG_TYPE",
      "createdAt": "2024-07-19T14:18:19.835699Z",
      "updatedAt": "2024-07-19T14:18:19.835699Z",
      "rules": [],
      "rollouts": [
        {
          "type": "THRESHOLD_ROLLOUT_TYPE",
          "rank": 1,
          "threshold": {
            "percentage": 50,
            "value": true
          }
        }
      ]
    },
    {
      "key": "dfsdf",
      "name": "dfsdf",
      "description": "",
      "enabled": true,
      "type": "VARIANT_FLAG_TYPE",
      "createdAt": "2024-07-19T14:20:48.359953Z",
      "updatedAt": "2024-07-21T16:59:59.373240Z",
      "rules": [],
      "rollouts": []
    }
  ]
}

@markphelps
Copy link
Collaborator Author

@erka @GeorgeMac this should be ready for another pass now

 into default-variant-redo

* 'default-variant-redo' of https://github.com/flipt-io/flipt:
  chore(deps): bump go.opentelemetry.io/otel/exporters/prometheus (#3290)
  chore(deps): bump golang.org/x/crypto from 0.24.0 to 0.25.0 (#3292)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/config (#3293)
  chore(deps): bump github.com/redis/go-redis/v9 from 9.5.3 to 9.6.0 (#3291)
  chore(deps-dev): bump playwright from 1.45.1 to 1.45.2 in /ui (#3297)
  chore(deps): bump chart.js from 4.4.2 to 4.4.3 in /ui (#3298)
  chore(deps-dev): bump eslint-plugin-no-relative-import-paths in /ui (#3295)
  chore(deps-dev): bump eslint-plugin-react from 7.34.4 to 7.35.0 in /ui (#3294)
  chore(deps): bump github.com/google/go-containerregistry (#3289)
Signed-off-by: Mark Phelps <[email protected]>
Signed-off-by: Mark Phelps <[email protected]>
@markphelps markphelps changed the title feat: default variant redo feat: default variants Jul 22, 2024
Copy link
Collaborator

@erka erka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks great. I have only one comment about compatibility.

internal/server/evaluation/data/server.go Show resolved Hide resolved
@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jul 23, 2024
@kodiakhq kodiakhq bot merged commit 97b0bc3 into main Jul 23, 2024
57 of 59 checks passed
@kodiakhq kodiakhq bot deleted the default-variant-redo branch July 23, 2024 11:41
markphelps added a commit that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Default Variant
3 participants