Restore logging of test configuration for pilot e2e tests#3459
Restore logging of test configuration for pilot e2e tests#3459istio-merge-robot merged 2 commits intoistio:masterfrom
Conversation
|
/lgtm |
|
Is the --logtostderr also worthwhile to restore? Offhand I thought misc tests were passing this (and circle still is in a few places) but since everybody was I think it was migrating towards this common location in the makefile. |
2af7f22 to
fc6665f
Compare
|
/lgtm cancel //PR changed after LGTM, removing LGTM. @ldemailly @nmittler |
fc6665f to
d7374e6
Compare
The CircleCI |
|
linter not happy ^ |
|
@nmittler PR needs rebase |
193529a to
e76a79b
Compare
|
@ldemailly should be good to go now ... mind re-approving? |
|
/retest |
|
@ldemailly any ideas why the tests are failing? Has something changed with the test environment? |
|
/test istio-pilot-e2e |
|
One aspect that probably did change on you was that KUEBCONFIG wasn't being set and that was fixed about a half day ago (the earlier successful test run for this PR logged "Env variable KUBECONFIG not set. Skipping tests"). |
e76a79b to
de8b847
Compare
|
@ldemailly PTAL ... I've broadened the scope of this PR to include several fixes to the pilot e2e tests |
|
/test istio-pilot-e2e |
|
this still seems to be a huge diff just to change 1 logging ? |
Agreed, there are a lot of changes, but it's mostly renames. See the updated description for an overview. The rename was effectively due the introduction of the new |
a496367 to
2be590b
Compare
|
@ldemailly gentle ping ... would like to get this in soonish if possible |
|
I asked Zach to have a glance As you will own this code I suppose it is fine you do whichever you feel comfortable but I wanted a second opinion |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
This was removed by istio#3502 To cleanly do this, I've separated out the configuration from the test runtime environment. I've renamed `Infra` to `Environment` to make this clear. Because fields are now split across `Config` and `Environment, I've also create a new `TemplateData` struct that merges all the common template parameters into a single struct. This way the template files do not have to change. The separation of template params from the environment should also help a bit with maintenance going forward.
2be590b to
609a97f
Compare
|
@ZackButcher I had to rebase ... mind re-approving? |
|
/lgtm cancel //PR changed after LGTM, removing LGTM. @ZackButcher @ldemailly @nmittler |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GregHanson, ldemailly, ZackButcher The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
This was removed by #3502
To cleanly do this, I've separated out the configuration from the
test runtime environment. I've renamed
InfratoEnvironmenttomake this clear.
Because fields are now split across
ConfigandEnvironment, I'vealso create a new
TemplateDatastruct that merges all the commontemplate parameters into a single struct. This way the template files
do not have to change. The separation of template params from the
environment should also help a bit with maintenance going forward.