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

MANTA-3052 want mahi to serve allowed_dcs attributes #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joyent-automation
Copy link

MANTA-3052 want mahi to serve allowed_dcs attributes

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/3968/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@rmustacc commented at 2018-12-15T01:50:23

Patch Set 1:

(3 comments)

@arekinath commented at 2018-12-17T22:44:33

Patch Set 1:

(3 comments)

@rmustacc commented at 2018-12-17T23:00:36

Patch Set 1: Code-Review+1

(2 comments)

Patch Set 1 code comments
lib/replicator/transforms/sdcperson.js#151 @rmustacc

So in the case where payload[type] is undefined/NULL this makes sense. But in other cases, does it make sense for us to drive on here? What if it's a string or object?

lib/replicator/transforms/sdcperson.js#151 @arekinath

This logic (which is copy-pasted from other attributes here) is, as I understand it, important to deal with the changelog entries output by some very old versions of UFDS. Having mahi ignore these seems fine, as they're not going to be data it cares about today (practically speaking), but we need to not blow up.

Basically all of this has to be pretty loosey goosey because UFDS itself is loosey goosey. If we try to be stricter than we currently are today in any one dimension on the data, I guarantee something violates it and will blow us up.

lib/replicator/transforms/sdcperson.js#151 @rmustacc

OK, fair enough.

lib/replicator/transforms/sdcperson.js#166 @rmustacc

Shouldn't there be a default case that blows up or handles this gracefully?

lib/replicator/transforms/sdcperson.js#166 @arekinath

See above. Handling a default case in a way differently to how we do it today for other attributes is fraught with peril.

lib/replicator/transforms/sdcperson.js#166 @rmustacc

OK, fair enough.

test/sdcperson.test.js#564 @rmustacc

This tests the case where we're adding entries that already exist and removing ones that don't exist. Probably worth doing the case where we're adding something that doesn't exist and removing something that does?

test/sdcperson.test.js#564 @arekinath

Fair, I'll add a test case.

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.

1 participant