Skip to content

Conversation

@duglin
Copy link

@duglin duglin commented Jan 8, 2019

Fixes #540

If people are ok with this change I'll make similar modifications to the other
instruction files too.

Signed-off-by: Doug Davis dug@us.ibm.com

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 8, 2019
@duglin duglin mentioned this pull request Jan 8, 2019
`istio.yaml` (which includes those CRD definitions) fails due to the CRD
controller not defining those types by the time we try to use them:
```bash
kubectl apply --filename https://github.com/knative/serving/releases/download/v0.2.2/istio-crds.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO at this point we should list this command like:

1 && \
  2 && \
  3

so that if people copy-paste all of this code block, it stops on failure.

Also I wonder if you still see errors when you don't add the sleep 10 (I feel like applying CRDs first is already a big improvement on the success rate).

Copy link
Author

@duglin duglin Jan 8, 2019

Choose a reason for hiding this comment

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

Will add the &&'s
And then test w/o the sleep....

@ahmetb
Copy link
Contributor

ahmetb commented Jan 8, 2019

Shouldn't this change be replicated on other cloud platforms as well? As far as I can tell, Istio installation race issues aren't specific to IKS.

@duglin
Copy link
Author

duglin commented Jan 8, 2019

Yep - I mentioned in the initial comment that once we get the text right, and people are ok with it, I can make the same change to the others.

@ahmetb
Copy link
Contributor

ahmetb commented Jan 8, 2019

Yep - I mentioned in the initial comment

sorry for not seeing it, my bad

Install the Istio CRDs first so that Kubernetes has time to create
the resource definitions. Sometimes the deployment of `istio.yaml`
(which includes those CRD definitions) fails because the types are
have not finished being defined by the CRD controller.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the istio.yaml deployment fails, what should you do? Wait and try again? If so, can we include that instruction?

Copy link
Author

Choose a reason for hiding this comment

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

Was wondering how verbose I should get on this w/o being annoying :-) But let me add something and then you guys can check it out....

Closes knative#540

If people are ok with this change I'll make similar modifications to the other
instruction files too.

Signed-off-by: Doug Davis <dug@us.ibm.com>
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 8, 2019
@duglin
Copy link
Author

duglin commented Jan 8, 2019

ok I think it did all of the updates. In my testing just pulling out the CRDs (so no "sleep") is sufficient. Please take a took and see if the more verbose text is ok.

@knative-prow-robot knative-prow-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 9, 2019
@duglin
Copy link
Author

duglin commented Jan 9, 2019

@carieshmarie did you push to my PR/branch? While I'm ok with your suggested change, I would prefer if you added it as a suggested edit so I can review it rather than to just push to my branch.

@carieshmarie
Copy link
Contributor

Sorry, @duglin - I meant to include a comment with an overview of what I was doing, but got sidelined. Apologies for the commit out of nowhere.

If you're ok with the change, then I think it's ready to be applied more broadly.

@carieshmarie carieshmarie added lgtm Indicates that a PR is ready to be merged. approved labels Jan 9, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: duglin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 5560ccb into knative:master Jan 9, 2019
@duglin
Copy link
Author

duglin commented Jan 9, 2019

No worries - will work on the other docs now...

@duglin duglin deleted the issue540 branch January 9, 2019 01:20
duglin pushed a commit to duglin/docs that referenced this pull request Jan 9, 2019
Continuation of knative#717 - get the rest of the install docs

Signed-off-by: Doug Davis <dug@us.ibm.com>
duglin pushed a commit to duglin/docs that referenced this pull request Jan 9, 2019
Continuation of knative#717 - get the rest of the install docs

Signed-off-by: Doug Davis <dug@us.ibm.com>
knative-prow-robot pushed a commit that referenced this pull request Jan 9, 2019
Continuation of #717 - get the rest of the install docs

Signed-off-by: Doug Davis <dug@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants