Conversation
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
I don't think we need a global here either. The code paths for the updater are pretty limited and I think they already receive the config.
We still want to be avoiding global as much as we reasonably can. The api PR is the exception, not the rule.
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
Looks good! Two minor comments, up to you what you do with them.
| e2e.OptAppendEnv(suite.env(false, false)...), | ||
| e2e.OptAppendEnv("VERBOSE=true"), | ||
| ) | ||
| cp.ExpectExitCode(0) |
There was a problem hiding this comment.
This really gives a zero exit code? I would imagine this will fail.
There was a problem hiding this comment.
The checker handles some 4xx codes without throwing an error. I think that's what's happening here.
| ts := e2e.New(suite.T(), false) | ||
| defer ts.Close() | ||
|
|
||
| cp := ts.Spawn("config", "set", constants.UpdateEndpointConfig, "https://example.com/update") |
There was a problem hiding this comment.
Nit: I prefer to use something that's not an actual domain like test.tld. I can only imagine the traffic example.com sees :p
There was a problem hiding this comment.
example.com is not a valid domain. This is so you can use it in these contexts.
There was a problem hiding this comment.
example.com is not a valid domain. This is so you can use it in these contexts.
I'm going to pretend that I knew that when I wrote this test 🙂
3 participants
Uh oh!
There was an error while loading. Please reload this page.