Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

e2e: Dump stdout and describe ksvc #659

Merged
merged 1 commit into from
Feb 12, 2020
Merged

e2e: Dump stdout and describe ksvc #659

merged 1 commit into from
Feb 12, 2020

Conversation

navidshaikh
Copy link
Collaborator

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 12, 2020
@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 12, 2020
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2020
@navidshaikh navidshaikh force-pushed the pr/dump-stdout branch 2 times, most recently from 694814b to ec056e7 Compare February 12, 2020 09:25
@navidshaikh
Copy link
Collaborator Author

/retest

ERROR: timeout waiting for pods to come up

@rhuss
Copy link
Contributor

rhuss commented Feb 12, 2020

/retest

@rhuss
Copy link
Contributor

rhuss commented Feb 12, 2020

@navidshaikh I think we can merged this PR regardless of we can catch the flake now. Shouldn't harm to have that extra output in case of an error ?

@navidshaikh
Copy link
Collaborator Author

@rhuss : would like to see once if its printing the required info before merging;

@navidshaikh
Copy link
Collaborator Author

navidshaikh commented Feb 12, 2020 •
edited
Loading

shows the stdout and describes ksvc

Creating service 'hello' in namespace 'kne2etests1':
  0.941s The Route is still working to reflect the latest desired specification.
  1.401s Revision "hello-hnzcf-1" referenced in traffic not found.

I think the issue is recorded in the event

Warning  InternalError  10m                service-controller  failed to reconcile Configuration: Operation cannot be fulfilled on configurations.serving.knative.dev "hello": the object has been modified; please apply your changes to the latest version and try again

see logs:
https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/659/pull-knative-client-integration-tests/1227550120803831808#1:build-log.txt%3A811

@navidshaikh
Copy link
Collaborator Author

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2020
@navidshaikh
Copy link
Collaborator Author

/retest

@rhuss
Copy link
Contributor

rhuss commented Feb 12, 2020

It's crazy, the test dont want to fail anymore ....

/retest

@navidshaikh
Copy link
Collaborator Author

@rhuss :

Error:

Failed to successfully execute 'kn service create hello --image gcr.io/knative-samples/helloworld-go -n kne2etests9': Execution error: stderr: 'timeout: service 'hello' not ready after 600 seconds

and

  Warning  InternalError  10m                service-controller  service: "hello" does not own configuration: "hello"

ref: logs

@navidshaikh
e2e: Dump stdout and describe ksvc
705f987 
 if the service create/update command execution failed
rhuss
rhuss approved these changes Feb 12, 2020
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh, rhuss

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 3019f68 into knative:master Feb 12, 2020
rhuss added a commit to rhuss/knative-client that referenced this pull request Feb 12, 2020
@rhuss
fix(service wait): Wrong check for error window
0eab471 
The error window introduced with knative#644 had a wrong conditional. Fixed that and added a test which would have detected this.

Also, this should fix some issues which we tried to detect with knative#659.
@rhuss rhuss mentioned this pull request Feb 12, 2020
Merged
@navidshaikh navidshaikh deleted the pr/dump-stdout branch February 13, 2020 06:50
knative-prow-robot pushed a commit that referenced this pull request Feb 13, 2020
@rhuss
fix(service wait): Wrong check for error window (#661)
52b56ba 
The error window introduced with #644 had a wrong conditional. Fixed that and added a test which would have detected this.

Also, this should fix some issues which we tried to detect with #659.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhuss rhuss rhuss approved these changes

Assignees

@rhuss rhuss

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@navidshaikh @rhuss @knative-prow-robot @googlebot