Related Documentation

Checked 3 published document(s). No updates required.

^{How did I do? Any feedback?}  

Codecov Report

❌ Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.74%. Comparing base (465e630) to head (0fa079e).
⚠️ Report is 11 commits behind head on v2.

❗ There is a different number of reports uploaded between BASE (465e630) and HEAD (0fa079e). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (465e630)	HEAD (0fa079e)
integrationtests	10	9
unittests	1	0

@@             Coverage Diff             @@
##               v2    #4732       +/-   ##
===========================================
- Coverage   62.49%   36.74%   -25.75%     
===========================================
  Files         132      124        -8     
  Lines       15554    14936      -618     
===========================================
- Hits         9720     5488     -4232     
- Misses       5153     8885     +3732     
+ Partials      681      563      -118     

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

Hey @causton81

Could you explain the issue you are trying to resolve and how is it related to streaming?

I think we need to simply add:

env, err := s.envs.Get(ctx, r.EnvironmentKey)
if err != nil {
	// try to get the environment from the context
	// this is for backwards compatibility with v1
	env = s.envs.GetFromContext(ctx)
}

like we do in

seems we missed getting the environment from the path parameters for streaming

Sorry for the lack of description. I added the error for the case where an environment is specified but not found, because it was confusing to get results and then not see my changes pushed out. It took me a little bit to notice I was getting the default environment instead of what I asked for. I was trying to allow for v1 requests with no environment, but I did not test that.

I think we need to simply add:

env, err := s.envs.Get(ctx, r.EnvironmentKey)
if err != nil {
	// try to get the environment from the context
	// this is for backwards compatibility with v1
	env = s.envs.GetFromContext(ctx)
}

like we do in

flipt/internal/server/evaluation/client/server.go

Lines 39 to 51 in 465e630

	func (s *Server) EvaluationSnapshotNamespace(ctx context.Context, r *rpcevaluation.EvaluationNamespaceSnapshotRequest) (*rpcevaluation.EvaluationNamespaceSnapshot, error) {
	start := time.Now()
	env, err := s.envs.Get(ctx, r.EnvironmentKey)
	if err != nil {
	// try to get the environment from the context
	// this is for backwards compatibility with v1
	env = s.envs.GetFromContext(ctx)
	}
	var (
	environmentKey = env.Key()
	namespaceKey = r.Key

seems we missed getting the environment from the path parameters for streaming

@markphelps should we use request path parameter only in streaming since it's always required to be there? It also remove any conflicts when path key and context key different if someone messes around with it.

Sorry for the lack of description. I added the error for the case where an environment is specified but not found, because it was confusing to get results and then not see my changes pushed out. It took me a little bit to notice I was getting the default environment instead of what I asked for. I was trying to allow for v1 requests with no environment, but I did not test that.

@causton81 I guess it would be better to change envs.GetFromContext to return error in this case. There are few places where this method is called and it would be nice to have the same API experience. wdyt @markphelps?

Sorry for the lack of description. I added the error for the case where an environment is specified but not found, because it was confusing to get results and then not see my changes pushed out. It took me a little bit to notice I was getting the default environment instead of what I asked for. I was trying to allow for v1 requests with no environment, but I did not test that.

@causton81 I guess it would be better to change envs.GetFromContext to return error in this case. There are few places where this method is called and it would be nice to have the same API experience. wdyt @markphelps?

yes I think that makes sense