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

Include Context information in config response #3916

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MGibson1
Copy link
Member

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [x] Other

Objective

Adds feature flag context to config response. This is useful for debugging purposes.

Question: Is there a reason to consider this sensitive? It uses the same bearer token to, say, retrieve full sync data, so all information is retrievable through other endpoints.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@withinfocus withinfocus marked this pull request as draft March 20, 2024 16:24
@MGibson1 MGibson1 marked this pull request as ready for review March 20, 2024 17:02
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.21%. Comparing base (03217e8) to head (61f26da).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3916      +/-   ##
==========================================
- Coverage   36.36%   36.21%   -0.16%     
==========================================
  Files        1158     1159       +1     
  Lines       56106    56134      +28     
  Branches     5385     5387       +2     
==========================================
- Hits        20405    20330      -75     
- Misses      34755    34859     +104     
+ Partials      946      945       -1     

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

@@ -1,5 +1,11 @@
namespace Bit.Core.Services;

public struct FeatureFlagContext
Copy link
Contributor

Choose a reason for hiding this comment

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

🎨 I am still hung up on this being associated to feature flags. My opinion is that context is a platform-level concept and should be there and not in these classes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thought, I was thinking of context in terms of the LD context, but that's entirely derived from CurrentContext. A safe report of that context should fully describe anything that any given Feature Flag provider could use to key off of

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants