feat(config): sync hook and mfa config to remote #2861
Conversation
Pull Request Test Coverage Report for Build 11741050509Details
💛 - Coveralls |
sweatybridge
force-pushed
the
sync-hook
branch
from
08040c7
to
6fe7019
Compare
sweatybridge
changed the title
| assert.Contains(t, string(diff), `[hook.mfa_verification_attempt]`) | ||
| assert.Contains(t, string(diff), `-enabled = true`) | ||
| assert.Contains(t, string(diff), `+enabled = false`) | ||
| assert.Contains(t, string(diff), `uri = ""`) | ||
| assert.Contains(t, string(diff), `secrets = ""`) | ||
|
|
||
| assert.Contains(t, string(diff), `[hook.custom_access_token]`) | ||
| assert.Contains(t, string(diff), `-enabled = false`) | ||
| assert.Contains(t, string(diff), `+enabled = true`) | ||
| assert.Contains(t, string(diff), `uri = ""`) |
There was a problem hiding this comment.
question
Is there an equivalent to toMatchInlineSnapshot in Go ? That could maybe be useful instead of repeating the contains for every line as it would also test for the lines order.
There was a problem hiding this comment.
Not that I'm aware of. But I'm interested as well.
A low hanging fruit is to write the expected diff output to a file under testdata for comparison.
| [auth.mfa.phone] | ||
| enroll_enabled = false | ||
| verify_enabled = false | ||
| otp_length = 6 | ||
| template = "Your code is {{ `{{ .Code }}` }}" | ||
| max_frequency = "5s" |
There was a problem hiding this comment.
question
Will it cause any issue if it stay commented instead of being explicitly set to false ? Wondering about retro-compatibility with existing config.
There was a problem hiding this comment.
One problem is if user sets verify_enabled = true without uncommenting the other fields, the update code would sync zero values to the api. I think that's rarely desired because it would override platform defaults like otp_length.
We can of course handle zero values in code, by treating it as null and not sending to api. But 0 is actually a valid config value according to auth team.
So I think it's cleaner to set non-zero default values for these fields to match our platform. If we want to support nullable fields in the future, we can also change them into pointers.
What kind of change does this PR introduce?
feature
What is the new behavior?
Sync
hook,mfa, andsessionsconfig to remote.Additional context
Add any other context or screenshots.