fwereadeniemeyer, response to https://codereview.appspot.com/6355077/ fwiw14:14
niemeyerfwereade: I'm almost done with the reply14:15
fwereadeniemeyer, cool :)14:16
niemeyerfwereade: Done14:16
niemeyerfwereade: Actually, we don't need Get and Set..14:21
niemeyerfwereade: If Validate wants to mutate the configuration, grab the map, do whatever, and return a new config14:21
niemeyerfwereade: This way a Config is immutable14:22
niemeyerfwereade: Which is a nice property and goes towards your desires14:22
fwereadeniemeyer, ok, that would probably work for me14:22
niemeyerfwereade: So ValidateConfig(old, new) => (valid *Config, err error)14:23
fwereadeniemeyer, yes indeed (sorry, laura is being obnoxious ;))14:29
niemeyerfwereade: It's Saturday, she's right! :p14:30
fwereadeniemeyer, nah, hitting me in the balls when I censured her for interrupting a conversation ;p14:31
niemeyerfwereade: ROTFL14:31
niemeyerfwereade: That's a bit extreme indeed ;-)14:31
fwereadeniemeyer, (I don't think it was deliberately targeted but that did not make it a notably more enjoyable experience ;))14:31
niemeyerfwereade: I know.. it's just the easiest target for her size14:32
fwereadeniemeyer, yeah :)14:32
fwereadeniemeyer, my only reservation remains that I don't really see on what basis you make the distinction between environ config and all the other things that we handled as dicts in python14:32
fwereadeniemeyer, I've been loving knowing exactly what's in the types and don;t really see any drawbacks :)14:33
niemeyerfwereade: I think you're too focused on how Python used a dict for the configuration14:33
niemeyerfwereade: The configuration *is* a dict14:33
niemeyerfwereade: That's not the issue per se14:33
niemeyerfwereade: and as I explained in the reply, you'll continue having a type in the exact places we have them today14:34
niemeyerfwereade: Nothing is changing14:34
niemeyerfwereade: We're just gaining in: a) Being able to carry that type around in a better form, in and out of state; b) Having a proper interface that reveals the common fields; c) Having a properly typed EnvironConfig in *more* places14:35
niemeyerfwereade: Right now we do provider.NewConfig(attrs).Open()14:35
niemeyerfwereade: We'll now do provider.Open(config)14:36
niemeyerfwereade: The typing happens inside the same path14:36
fwereadeniemeyer, that is indeed surely nicer14:36
fwereadeniemeyer, I think the specific thing I am paranoid about is that State.SetEnvironConfig will be unable to do appropriate replacement validation, and I prefer to mistrust my clients (even when I myself am writing the client ;))14:37
niemeyerfwereade: I have the same concern, and I see the solution I'm suggesting as doing a better job in exact that regard14:38
niemeyerfwereade: SetEnvironConfig is too late for doing validation.. we shouldn't allow an invalid config to ever reach it14:39
niemeyerfwereade: The proper place to do configuration validation is "at the door"14:39
fwereadeniemeyer, I totally agree there -- it's just that, well, bad things happen and stupid code slips in, and I'm twitchy about being unable to do sanity-checking in the state package14:44
fwereadeniemeyer, offhand we want to set env state (1) at Initialize time, (2) at env-set time, (3) (probably) when setting constraints when we get to them and (4) on first secure connection, for the secrets14:46
fwereadeniemeyer, the burden of putting the sanity-checking in each of those places is I agree not overwhelming14:46
fwereadeniemeyer, but my heart says that I should be able to fling anything I want at State.SetEnvironConfig and have it dourly block crazy requests14:47
niemeyerfwereade: Man.. do you want me to implement that?14:49
niemeyerfwereade: This is trivial really14:49
niemeyerfwereade: We only need a single function that does validation of configuration at entrance14:49
niemeyerfwereade: This is getting totally out of proportion to the actual problem14:50
fwereadeniemeyer, sorry, I'm not even really arguing against your approach any more -- I'm mostly just trying to figure out where I'm being crazy14:50
niemeyerfwereade: The root of the problem is that you're obsessed about doing validation at SetEnvironConfig, and can't see that it's trivial to do validation in *one single place* elsewhere14:51
niemeyerfwereade: That should be the place where we get EnvironConfig from..14:55
niemeyerfwereade: If the way we use to load EnvironConfig validates it, how can we possibly get an EnvironConfig into State.SetEnvironConfig that is invalid?14:55
fwereadeniemeyer, ok, I appear to have missed something... the Validate method you propose takes a *Config14:57
fwereadeniemeyer, well, two, but I thought you were saying we should be calling ValidateConfig(nil, someConfig)14:58
niemeyerfwereade: Yes, that's the interface through which the *provider* validates the config14:58
niemeyerfwereade: Who calls it?14:58
niemeyerfwereade: How do we get from yaml onto a *Config?14:58
fwereadeniemeyer, ok; but that is not validating at the gate14:58
niemeyerfwereade: No, it's not, it's infrastructure to do what must be done14:59
niemeyerfwereade: We can have something within environs itself that validates it at the door14:59
niemeyerfwereade: Leave Read there, for example14:59
niemeyerfwereade: and run ValidateConfig on all configuration we get from environments.yaml15:00
fwereadeniemeyer, ok, *that* SGTM15:00
fwereadeniemeyer, it seemed to me that I had propsed something that validated at the gate and you were saying we didn't need that15:01
niemeyer<niemeyer> fwereade: We only need a single function that does validation of configuration at entrance15:02
fwereadeniemeyer, (I'm not *specifically* obsessed with validating in state -- more with being sure that if I see a *Config I can know is actually valid)15:02
niemeyer<niemeyer> fwereade: The proper place to do configuration validation is "at the door"15:02
niemeyerfwereade: It seems like I've been repeating that since yesterday15:02
fwereadeniemeyer, yeah, and your proposed solution did not allow for that -- but I think moving Read/ReadBytes does15:03
niemeyerfwereade: How did it not allow hat?15:03
fwereadeniemeyer, I guess I'm missing something still... how did it allow for it?15:04
niemeyerfwereade: Can you please tell me what do you see as a blocker?15:05
niemeyerfwereade: I've been saying the same thing, so maybe if you tell me what's the limitation I can fix my explanation, or figure what I'm thinking wrong15:06
fwereadeniemeyer, it seemed to be explicitly allowing for unvalidated *Configs to be floating around...15:06
fwereadeniemeyer, I think I am happy with your latest suggestion15:06
niemeyerfwereade: "Floating around" is somewhat unprecise15:06
niemeyerfwereade: ValidateConfig does validation, it takes two *Config values15:07
niemeyerfwereade: So, necessarily, it is possible for an unvalidated *Config to exist before it is validated15:07
niemeyerfwereade: We do validation as soon as possible15:07
fwereadeniemeyer, but what you suggested yesterday AFAICT indicated that you expected people to get *Configs from the config package, and to make sure they used ValidateConfig from the environs package, before they did anything with that *Config15:07
niemeyerfwereade: That's been the plan I'm trying to suggest since I first suggested a proper EnvironConfig type rather than interface15:08
niemeyerfwereade: Yes, that's what I'm still suggesting15:08
niemeyerfwereade: I also said we should verify as soon as possible15:08
niemeyerfwereade: Using that same mechanism15:08
fwereadeniemeyer, huh, OK, I thought that by moving Read into environs we could guarantee that nobody saw a *Config that hadn't passed through Validate15:09
niemeyerfwereade: YES!15:09
niemeyerfwereade: Those two things do not conflict!15:09
niemeyerfwereade: "As soon as possible" may involve Read, and whatever15:09
fwereadeniemeyer, ...but *without* Read in environs, we surely make it a lot easier to forget to validate?15:10
fwereadeniemeyer, hence my discomfort15:10
fwereadeniemeyer, anyway, thank you for bearing with me15:11
niemeyerfwereade: Sure, it was my mistake to suggest that in environs/config in the paste.. I clearly didn't think through15:11
niemeyerfwereade: This has always been my feeling, though, and I think you'll agree with me if you read back15:11
fwereadeniemeyer, yeah, it was the code I was disagreeing with15:12
fwereadeniemeyer, I had a strong feeling of us somehow being in violent agreement15:13
niemeyerfwereade: Phew, glad to hear it :)15:13
fwereadeniemeyer, I think I conceded the validate-at-the-gate point yesterday, and it seemed to me that reading via environs was the only way to do it15:14
niemeyerfwereade: Well, I hope we do understand each other15:18
niemeyerfwereade: Because I'm still suggesting an EnvironConfig in the form that is in the paste15:18
niemeyerfwereade: and the use of environs/config for having it, so that we can use in State et al15:18
niemeyerfwereade: Reading it, and validating it, can be done within environs, though15:19
niemeyerfwereade: Is that what you understood from the agreemnet as well?15:21
fwereadeniemeyer, the only tweak I would favour is to put the common fields in as fields and keep the provider-specific types in a map, but even that is not worth the time we'd spend debating (unless you immediately think it's a good idea ;))15:21
fwereadegahh not provider-specific *types*15:21
fwereadeprovider-specific attributes/settings/whatever-we-call-them15:22
niemeyerfwereade: I don't think it's a good idea only because state needs the full map15:22
fwereadeniemeyer, we're copying the map anyway15:22
fwereadeniemeyer, but I concede :)15:22
niemeyerfwereade: Well, I don't understand the suggestion then.. you suggested having only provider-specific types in the map, I said state needs the full map, you say ... ?15:24
niemeyerfwereade: I'd be happy with having an additional method, like CustomMap(), that excludes the well known fields15:25
niemeyerfwereade: I don't know if that's what you're suggesting?15:25
fwereadeniemeyer, I say "we'll be implementing Map such that it copies, anyway, and the 5 lines of code to explicitly poke in the common field values is not going to be a ig deal"15:25
niemeyerfwereade: It sounds like too much trouble for little reason..15:26
fwereadeniemeyer, like I said I don;t think the benefits outweigh the costs of talking about it15:26
niemeyerfwereade: c.m.Get("default-series").(string) vs. 1) Do the same once; 2) Augment the struct; 3) Reinject the fields15:27
niemeyerfwereade: Sure, sorry..15:27
niemeyerfwereade: Something like Extras() seems like a good idea, though15:27
niemeyerfwereade: Returning a map of unknown fields15:27
fwereadeniemeyer, oh, wait, one more thought... Secrets are an env-specific subset of Extras15:27
niemeyerfwereade: Yeah15:27
fwereadeniemeyer, meh, we'll figure something out15:27
fwereadeniemeyer, it's certainly not enough to invalidate your suggestion15:28
niemeyerfwereade: I think provider.ConfigSecrets(config) => <somethign> could do it15:28
fwereadeniemeyer, we also want to be able to strip them out, but nowhere we want to do that won;t have a real Environ anyway15:29
fwereadeniemeyer, there are certainly no architectural issues with it15:29
* fwereade is happy15:32
fwereadeniemeyer, thanks again for spending your weekend on this ;)15:33
niemeyerfwereade: No problem, and sorry for being unable to articulate the idea in a clearer way before15:33
fwereadeniemeyer, it takes 2 to have a communication problem ;)15:34
niemeyerfwereade: Thanks :-)15:34
niemeyerfwereade: Heading out for lunch.. have a nice weekend in case we don't speak today again15:56

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!