Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Refine Istio example #2819

Merged
merged 5 commits into from
May 3, 2018
Merged

Refine Istio example #2819

merged 5 commits into from
May 3, 2018

Conversation

billpratt
Copy link
Contributor

Addressing #2569

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, @dtzar do you wanna take a look before we merge?

@dtzar
Copy link
Contributor

dtzar commented May 1, 2018

LGTM, with some minor comments. I talked with @billpratt during sync week last week where their focus was this topic area. It seems 0.7.0 Istio and/or updates to K8s has resolved some problems I encountered at the time I submitted the first example - in particular, the previous need to specify the Intializers flag to make automatic sidecar injection.

"orchestratorRelease": "1.9",
"kubernetesConfig": {
"apiServerConfig": {
"--admission-control": "NamespaceLifecycle,LimitRanger,ServiceAccount,DefaultStorageClass,DefaultTolerationSeconds,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota,DenyEscalatingExec,AlwaysPullImages,ValidatingAdmissionWebhook,ResourceQuota",
Copy link
Contributor

@dtzar dtzar May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encountered problems with some of the bookinfo examples deploying properly when you use DenyEscalatingExec and AlwaysPullImages. Did you test this out? If it causes errors, it might be good to at least mention this so people can consider to enable them or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had no problems leaving these enabled.

### Installation

#### Create Azure Resources

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, I would leave this stuff for the main walkthrough so it doesn't have to be maintained.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wesyao Since you wrote these changes, what are your thoughts?

Copy link

@wesyao wesyao May 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackfrancis @dtzar @billpratt Sorry about the delay, I was waiting for others to chime in. I think it would be good to have now and we can remove this when it is repetitive? What do you guys think?

@billpratt
Copy link
Contributor Author

@jackfrancis some checks seem to be stuck?

@jackfrancis
Copy link
Member

@billpratt I was waiting on a response from @wesyao, but let's proceed, we can address @dtzar's comment in a follow-up PR if there's agreement.

And btw because this is just markdown + json maintenance we don't have to run E2E.

@jackfrancis jackfrancis merged commit 897fbbc into Azure:master May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants