Skip to content

Surface deployment replica failure status#4136

Closed
edwardstudy wants to merge 1 commit intoknative:masterfrom
edwardstudy:ed/issue-496
Closed

Surface deployment replica failure status#4136
edwardstudy wants to merge 1 commit intoknative:masterfrom
edwardstudy:ed/issue-496

Conversation

@edwardstudy
Copy link
Copy Markdown

Fixes #496

Proposed Changes

  • Following spec, surfaced deployment replica failure due ti insufficient quota, limit ranges, etc.

Release Note

NONE

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 21, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 21, 2019
Copy link
Copy Markdown
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@edwardstudy: 1 warning.

Details

In response to this:

Fixes #496

Proposed Changes

  • Following spec, surfaced deployment replica failure due ti insufficient quota, limit ranges, etc.

Release Note

NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Comment thread pkg/apis/serving/v1alpha1/revision_types.go Outdated
@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

Hi @edwardstudy. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added the area/API API objects and controllers label May 21, 2019
@vagababov
Copy link
Copy Markdown
Contributor

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2019
@markusthoemmes
Copy link
Copy Markdown
Contributor

@edwardstudy sorry that this has not yet gotten any reviews. Do you mind rebasing this PR? I vaguely recall a slew of PRs in similar vein, so maybe check if this is still needed?

@knative-prow-robot knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Jul 2, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: edwardstudy
To complete the pull request process, please assign mattmoor
You can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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

Details 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

@edwardstudy
Copy link
Copy Markdown
Author

@markusthoemmes Hi, thx for reply. I rebased.

This PR is surfacing a condition about underlying Deployment issues not logging stuff.

Copy link
Copy Markdown
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

/lint

Copy link
Copy Markdown
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@vagababov: 1 warning.

Details

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

revCondSet.Manage(rs).MarkFalse(RevisionConditionResourcesAvailable, "ProgressDeadlineExceeded", message)
}

func (rs *RevisionStatus) MarkNoDeployment(message string) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Golint comments: exported method RevisionStatus.MarkNoDeployment should have comment or be unexported. More info.

@edwardstudy edwardstudy force-pushed the ed/issue-496 branch 3 times, most recently from c366ecd to 88f53fc Compare July 4, 2019 08:17
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/serving/v1alpha1/revision_lifecycle.go 78.2% 78.6% 0.4

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@edwardstudy: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-knative-serving-go-coverage 613ca33 link /test pull-knative-serving-go-coverage
pull-knative-serving-unit-tests 613ca33 link /test pull-knative-serving-unit-tests
pull-knative-serving-build-tests 613ca33 link /test pull-knative-serving-build-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

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

Another rebase required I fear.

/assign @jonjohnsonjr

I think you've been looking into this in detail?

@markusthoemmes
Copy link
Copy Markdown
Contributor

@edwardstudy any news?

@markusthoemmes
Copy link
Copy Markdown
Contributor

Going to naively close this. @edwardstudy please reopen this if you want to rebase it and pick it back up. I'm not sure how much is left after @jonjohnsonjr recent PRs.

/close

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@markusthoemmes: Closed this PR.

Details

In response to this:

Going to naively close this. @edwardstudy please reopen this if you want to rebase it and pick it back up. I'm not sure how much is left after @jonjohnsonjr recent PRs.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Surface Revision quota problems

7 participants