-
Notifications
You must be signed in to change notification settings - Fork 667
GitOps: Add error handling (#1278) #10468
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.
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
GitOps: Add error handling (#1278) #10468
Conversation
1491043 to
961eb01
Compare
|
/retest |
|
Hi @debsmita1, @karthikjeeyar, while the tests are being looked at, could you review this? We'd like to get this completed for this sprint. |
|
Hi @vikram-raj could you please review this as well? |
|
/retest |
frontend/packages/gitops-plugin/src/components/GitOpsDetailsPage.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/gitops-plugin/src/components/details/GitOpsDetails.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/gitops-plugin/src/components/details/GitOpsDetails.tsx
Outdated
Show resolved
Hide resolved
|
Hi @Preeticp, this is a bug fix to better handle error conditions. Do we typically add documentation to these types of fixes? If not, the I think we can add the label for approving docs. |
|
@RickJWagner , I think we need the px-approved label on this one. |
|
@keithchong It seems you havent pushed the changes. |
961eb01 to
a1a83de
Compare
|
Hi @divyanshiGupta , indeed. It's pushed now. |
|
@keithchong you need to resolve the conflicts. |
|
Yeah, thanks @divyanshiGupta , it's because PR 10500 was merged. I also get the feeling I need to rebase again once 10420 is merged, which should be done first. |
a1a83de to
f7d7b22
Compare
|
/retest |
|
/label px-approved |
|
/retest |
|
/lgtm |
|
Hi @varshab1210 , as QE rep, could you please approve this? |
|
/label qe-approved |
|
@keithchong: The label(s) DetailsIn response to this:
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. |
rohitkrai03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Signed-off-by: Keith Chong <kykchong@redhat.com>
c4ab7a4 to
2cc0a07
Compare
|
Hi @divyanshiGupta , could you please re-add the lgtm label. I had to rebase because another PR was just merged. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, divyanshiGupta, keithchong, rohitkrai03 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 |
|
@keithchong: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
See GitOps Jira 1278
Created PR for now, but #10420 should be merged first. Looks like only one file needs to be rebased, so I'll do this change after.
Signed-off-by: Keith Chong kykchong@redhat.com