feat(targets): Addition of egress and ingress worker filters #2654
Conversation
irenarindos
changed the title
github-actions
bot
added
api
api/targets
core
core/daemon
core/db
core/gen
core/proto
core/session
core/sql
core/target
labels
| // Update the old worker filter | ||
| newWorkerFilter := "bar==foo" | ||
| result.SetWorkerFilter(newWorkerFilter) | ||
| result, _, _, _, err = testRepo.UpdateTarget(ctx, result, result.GetVersion(), []string{"WorkerFilter"}) | ||
| require.Equal(t, newWorkerFilter, result.GetWorkerFilter()) |
There was a problem hiding this comment.
I like grouping these cases like this. Consider using t.Run("Update the old worker filter", func(t testing.T) {... instead of using comments so what is being tested can be surfaced in the test logs and users don't have to go code diving to see which assertions are failing.
| if new.worker_filter = old.worker_filter then | ||
| new.worker_filter = null; |
There was a problem hiding this comment.
I think we've discussed this in the past but I just want to make sure the decision was made that we would clear the old worker_filter for the user instead of making them clear it manually.
This is potentially surprising behavior since they did not explicitly say they wanted it cleared. Alternatively if they have to explicitly clear it they would have a 2 step migration process of clearing worker_filter and then removing the clearing of the worker_filter, which may or may not be better.
There was a problem hiding this comment.
We went back and forth on it but decided to clear the old filter instead of making them clear it manually, some of that discussion is in the thread here: https://hashicorp.slack.com/archives/C038L63F67L/p1667242927405219
| @@ -64,7 +64,7 @@ func TestTargets(t *testing.T) { | |||
| }, | |||
| SessionMaxSeconds: &wrapperspb.UInt32Value{Value: 0}, | |||
| SessionConnectionLimit: &wrapperspb.Int32Value{Value: 0}, | |||
| WorkerFilter: &wrapperspb.StringValue{Value: "worker-filter"}, | |||
| EgressWorkerFilter: &wrapperspb.StringValue{Value: "egress-worker-filter"}, | |||
There was a problem hiding this comment.
Do we actually want to remove WorkerFilter? Aren't we still returning that for a while?
internal/db/schema/migrations/oss/postgres/59/01_target_ingress_egress_worker_filters.up.sql
Show resolved
Hide resolved
Partner PR to https://github.com/hashicorp/boundary-enterprise/pull/164