| fwereade | niemeyer, response to https://codereview.appspot.com/6355077/ fwiw | 14:14 |
|---|---|---|
| niemeyer | fwereade: I'm almost done with the reply | 14:15 |
| fwereade | niemeyer, cool :) | 14:16 |
| niemeyer | fwereade: Done | 14:16 |
| niemeyer | fwereade: Actually, we don't need Get and Set.. | 14:21 |
| niemeyer | fwereade: If Validate wants to mutate the configuration, grab the map, do whatever, and return a new config | 14:21 |
| niemeyer | fwereade: This way a Config is immutable | 14:22 |
| niemeyer | fwereade: Which is a nice property and goes towards your desires | 14:22 |
| fwereade | niemeyer, ok, that would probably work for me | 14:22 |
| niemeyer | fwereade: So ValidateConfig(old, new) => (valid *Config, err error) | 14:23 |
| fwereade | niemeyer, yes indeed (sorry, laura is being obnoxious ;)) | 14:29 |
| niemeyer | fwereade: It's Saturday, she's right! :p | 14:30 |
| fwereade | niemeyer, nah, hitting me in the balls when I censured her for interrupting a conversation ;p | 14:31 |
| niemeyer | fwereade: ROTFL | 14:31 |
| niemeyer | fwereade: That's a bit extreme indeed ;-) | 14:31 |
| fwereade | niemeyer, (I don't think it was deliberately targeted but that did not make it a notably more enjoyable experience ;)) | 14:31 |
| niemeyer | fwereade: I know.. it's just the easiest target for her size | 14:32 |
| fwereade | niemeyer, yeah :) | 14:32 |
| fwereade | niemeyer, 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 python | 14:32 |
| fwereade | niemeyer, I've been loving knowing exactly what's in the types and don;t really see any drawbacks :) | 14:33 |
| niemeyer | fwereade: I think you're too focused on how Python used a dict for the configuration | 14:33 |
| niemeyer | fwereade: The configuration *is* a dict | 14:33 |
| niemeyer | fwereade: That's not the issue per se | 14:33 |
| niemeyer | fwereade: and as I explained in the reply, you'll continue having a type in the exact places we have them today | 14:34 |
| niemeyer | fwereade: Nothing is changing | 14:34 |
| niemeyer | fwereade: 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* places | 14:35 |
| niemeyer | fwereade: Right now we do provider.NewConfig(attrs).Open() | 14:35 |
| niemeyer | fwereade: We'll now do provider.Open(config) | 14:36 |
| niemeyer | fwereade: The typing happens inside the same path | 14:36 |
| fwereade | niemeyer, that is indeed surely nicer | 14:36 |
| fwereade | niemeyer, 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 |
| niemeyer | fwereade: I have the same concern, and I see the solution I'm suggesting as doing a better job in exact that regard | 14:38 |
| niemeyer | fwereade: SetEnvironConfig is too late for doing validation.. we shouldn't allow an invalid config to ever reach it | 14:39 |
| niemeyer | fwereade: The proper place to do configuration validation is "at the door" | 14:39 |
| fwereade | niemeyer, 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 package | 14:44 |
| fwereade | niemeyer, 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 secrets | 14:46 |
| fwereade | niemeyer, the burden of putting the sanity-checking in each of those places is I agree not overwhelming | 14:46 |
| fwereade | niemeyer, but my heart says that I should be able to fling anything I want at State.SetEnvironConfig and have it dourly block crazy requests | 14:47 |
| niemeyer | fwereade: Man.. do you want me to implement that? | 14:49 |
| niemeyer | fwereade: This is trivial really | 14:49 |
| niemeyer | fwereade: We only need a single function that does validation of configuration at entrance | 14:49 |
| niemeyer | fwereade: This is getting totally out of proportion to the actual problem | 14:50 |
| fwereade | niemeyer, sorry, I'm not even really arguing against your approach any more -- I'm mostly just trying to figure out where I'm being crazy | 14:50 |
| niemeyer | fwereade: 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* elsewhere | 14:51 |
| niemeyer | fwereade: That should be the place where we get EnvironConfig from.. | 14:55 |
| niemeyer | fwereade: 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 |
| fwereade | niemeyer, ok, I appear to have missed something... the Validate method you propose takes a *Config | 14:57 |
| fwereade | niemeyer, well, two, but I thought you were saying we should be calling ValidateConfig(nil, someConfig) | 14:58 |
| niemeyer | fwereade: Yes, that's the interface through which the *provider* validates the config | 14:58 |
| niemeyer | fwereade: Who calls it? | 14:58 |
| niemeyer | fwereade: How do we get from yaml onto a *Config? | 14:58 |
| fwereade | niemeyer, ok; but that is not validating at the gate | 14:58 |
| niemeyer | fwereade: No, it's not, it's infrastructure to do what must be done | 14:59 |
| niemeyer | fwereade: We can have something within environs itself that validates it at the door | 14:59 |
| niemeyer | fwereade: Leave Read there, for example | 14:59 |
| niemeyer | fwereade: and run ValidateConfig on all configuration we get from environments.yaml | 15:00 |
| fwereade | niemeyer, ok, *that* SGTM | 15:00 |
| fwereade | niemeyer, it seemed to me that I had propsed something that validated at the gate and you were saying we didn't need that | 15:01 |
| niemeyer | <niemeyer> fwereade: We only need a single function that does validation of configuration at entrance | 15:02 |
| fwereade | niemeyer, (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 |
| niemeyer | fwereade: It seems like I've been repeating that since yesterday | 15:02 |
| fwereade | niemeyer, yeah, and your proposed solution did not allow for that -- but I think moving Read/ReadBytes does | 15:03 |
| niemeyer | fwereade: How did it not allow hat? | 15:03 |
| niemeyer | that | 15:03 |
| fwereade | niemeyer, I guess I'm missing something still... how did it allow for it? | 15:04 |
| niemeyer | fwereade: Can you please tell me what do you see as a blocker? | 15:05 |
| niemeyer | fwereade: 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 wrong | 15:06 |
| fwereade | niemeyer, it seemed to be explicitly allowing for unvalidated *Configs to be floating around... | 15:06 |
| fwereade | niemeyer, I think I am happy with your latest suggestion | 15:06 |
| niemeyer | fwereade: "Floating around" is somewhat unprecise | 15:06 |
| niemeyer | fwereade: ValidateConfig does validation, it takes two *Config values | 15:07 |
| niemeyer | fwereade: So, necessarily, it is possible for an unvalidated *Config to exist before it is validated | 15:07 |
| niemeyer | fwereade: We do validation as soon as possible | 15:07 |
| fwereade | niemeyer, 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 *Config | 15:07 |
| niemeyer | fwereade: That's been the plan I'm trying to suggest since I first suggested a proper EnvironConfig type rather than interface | 15:08 |
| niemeyer | fwereade: Yes, that's what I'm still suggesting | 15:08 |
| niemeyer | fwereade: I also said we should verify as soon as possible | 15:08 |
| niemeyer | fwereade: Using that same mechanism | 15:08 |
| fwereade | niemeyer, huh, OK, I thought that by moving Read into environs we could guarantee that nobody saw a *Config that hadn't passed through Validate | 15:09 |
| niemeyer | fwereade: YES! | 15:09 |
| niemeyer | fwereade: Those two things do not conflict! | 15:09 |
| niemeyer | :-) | 15:09 |
| niemeyer | fwereade: "As soon as possible" may involve Read, and whatever | 15:09 |
| fwereade | niemeyer, ...but *without* Read in environs, we surely make it a lot easier to forget to validate? | 15:10 |
| fwereade | niemeyer, hence my discomfort | 15:10 |
| fwereade | niemeyer, anyway, thank you for bearing with me | 15:11 |
| niemeyer | fwereade: Sure, it was my mistake to suggest that in environs/config in the paste.. I clearly didn't think through | 15:11 |
| niemeyer | fwereade: This has always been my feeling, though, and I think you'll agree with me if you read back | 15:11 |
| fwereade | niemeyer, yeah, it was the code I was disagreeing with | 15:12 |
| fwereade | niemeyer, I had a strong feeling of us somehow being in violent agreement | 15:13 |
| niemeyer | fwereade: Phew, glad to hear it :) | 15:13 |
| fwereade | niemeyer, 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 it | 15:14 |
| niemeyer | fwereade: Well, I hope we do understand each other | 15:18 |
| niemeyer | fwereade: Because I'm still suggesting an EnvironConfig in the form that is in the paste | 15:18 |
| niemeyer | fwereade: and the use of environs/config for having it, so that we can use in State et al | 15:18 |
| niemeyer | fwereade: Reading it, and validating it, can be done within environs, though | 15:19 |
| niemeyer | fwereade: Is that what you understood from the agreemnet as well? | 15:21 |
| fwereade | niemeyer, 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 |
| fwereade | gahh not provider-specific *types* | 15:21 |
| fwereade | provider-specific attributes/settings/whatever-we-call-them | 15:22 |
| niemeyer | fwereade: I don't think it's a good idea only because state needs the full map | 15:22 |
| fwereade | niemeyer, we're copying the map anyway | 15:22 |
| fwereade | niemeyer, but I concede :) | 15:22 |
| niemeyer | fwereade: 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 |
| niemeyer | fwereade: I'd be happy with having an additional method, like CustomMap(), that excludes the well known fields | 15:25 |
| niemeyer | fwereade: I don't know if that's what you're suggesting? | 15:25 |
| fwereade | niemeyer, 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 |
| niemeyer | fwereade: It sounds like too much trouble for little reason.. | 15:26 |
| fwereade | niemeyer, like I said I don;t think the benefits outweigh the costs of talking about it | 15:26 |
| fwereade | ;) | 15:26 |
| niemeyer | fwereade: c.m.Get("default-series").(string) vs. 1) Do the same once; 2) Augment the struct; 3) Reinject the fields | 15:27 |
| niemeyer | fwereade: Sure, sorry.. | 15:27 |
| niemeyer | fwereade: Something like Extras() seems like a good idea, though | 15:27 |
| niemeyer | fwereade: Returning a map of unknown fields | 15:27 |
| fwereade | niemeyer, oh, wait, one more thought... Secrets are an env-specific subset of Extras | 15:27 |
| niemeyer | fwereade: Yeah | 15:27 |
| fwereade | niemeyer, meh, we'll figure something out | 15:27 |
| fwereade | niemeyer, it's certainly not enough to invalidate your suggestion | 15:28 |
| niemeyer | fwereade: I think provider.ConfigSecrets(config) => <somethign> could do it | 15:28 |
| fwereade | niemeyer, we also want to be able to strip them out, but nowhere we want to do that won;t have a real Environ anyway | 15:29 |
| fwereade | niemeyer, there are certainly no architectural issues with it | 15:29 |
| * fwereade is happy | 15:32 | |
| fwereade | niemeyer, thanks again for spending your weekend on this ;) | 15:33 |
| niemeyer | fwereade: No problem, and sorry for being unable to articulate the idea in a clearer way before | 15:33 |
| fwereade | niemeyer, it takes 2 to have a communication problem ;) | 15:34 |
| niemeyer | fwereade: Thanks :-) | 15:34 |
| niemeyer | fwereade: Heading out for lunch.. have a nice weekend in case we don't speak today again | 15:56 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!