Fix gometalinter 3.0 errors and update e2e bringup with istio/istio helm changes#88
Conversation
- per gometalinter 3.0 notes, replace megacheck with staticcheck - fix errors found by staticcheck (mostly unused funcs/vars)
tiswanso
force-pushed
the
lint_fix
branch
3 times, most recently
from
0d556ed to
7e45fb3
Compare
.circleci/run_e2e.sh
Outdated
|
|
||
| helm template --values ${chartdir}/istio-cni/values.yaml --name=istio-cni --namespace=istio-system --set "excludeNamespaces={}" --set hub=${CNI_HUB} --set tag=${CNI_TAG} --set pullPolicy=IfNotPresent --set logLevel=${CNI_LOGLVL:-debug} ${chartdir}/istio-cni > istio-cni_install.yaml | ||
|
|
||
| kubectl create ns istio-system |
There was a problem hiding this comment.
Why are you creating the ns here. I don't think that should be required.
There was a problem hiding this comment.
All the changes in this file are WIP. Trying to get the process working. Possibly this should go into istio/istio under a check for enable istio-cni.
But the create ns needs to be done prior to installing the istio-cni daemonset in that ns.
.circleci/run_e2e.sh
Outdated
|
|
||
| HUB=${HUB} TAG=${TAG} make istioctl | ||
|
|
||
| # Remove any pre-existing charts |
There was a problem hiding this comment.
You should remove all istio-cni.X helm settings since the istio-cni chart was removed in 1.1 and presumably will eventually be removed from master. Also since you are installing cni above if either master or the release still has istio-cni integrated things are likely to break. Note also as best I can tell master has not removed the subchart so that is going to make it difficult to test.
There was a problem hiding this comment.
yeah... the extra helm settings for istio-cni.X will all change. They don't hurt so I didn't touch the line. This file diff is WIP for now.
| @@ -3,11 +3,24 @@ | |||
| CNI_HUB=${CNI_HUB:-istio} | |||
There was a problem hiding this comment.
Is this a good default value
tiswanso
changed the title
|
NOTE: e2e-dind won't work until istio/istio master gets the Istio release1-1 helm charts change. |
.circleci/run_e2e.sh
Outdated
| # Install istio-cni prior to executing the Istio e2e test. Now that the helm chart for istio/istio no longer | ||
| # depends on the istio-cni chart, we need to explicitly do these steps. | ||
| helm init --client-only | ||
| #helm repo add istio.io https://storage.googleapis.com/istio-release/releases/1.1.0-snapshot.6/charts |
There was a problem hiding this comment.
I think you should remove the comments here. Thinking about this more the tests here should be only about using the istio source code not also validating if the the CNI charts have been properly pushed to the istio charts repo. The tests on the istio side can confirm that. In this repo it is probably cleaner if it tests the CNI code AND the CNI charts at the tip of the branch but pull in the Istio code to confirm that.
There was a problem hiding this comment.
Is this comment just to remove lines 13 & 14 or to remove all the other comments above? Lines 10 & 11 are still valid comments. I could reword to explain that we're testing with the istio-cni chart from the PR under-test.
john-a-joyce
left a comment
john-a-joyce
left a comment
There was a problem hiding this comment.
Per the comment I would prefer you remove the comments (and as extra credit comment on the strategy of using the local charts) and we stick to that going forward. Since it is just comment related changes I have no issues if you want to merge as is.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
Uh oh!
There was an error while loading. Please reload this page.