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.

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

No operator action required issues #1040

Merged
merged 8 commits into from
Jun 25, 2021
Merged

No operator action required issues #1040

merged 8 commits into from
Jun 25, 2021

Conversation

grolu
Copy link
Contributor

@grolu grolu commented Jun 22, 2021

What this PR does / why we need it:
This PR adds support for the new ERR_INFRA_REQUEST_THROTTLING error code.
Furthermore the option to hide user issues for operators has been replaced by an option to remove both user issues and temporary issues.

Screenshot 2021-06-22 at 17 40 25

Which issue(s) this PR fixes:
Fixes #1031 Fixes #1046

Special notes for your reviewer:
@vpnachev Please let me know if this change makes sense from your point of view.

Release note:

Added support for `ERR_RETRYABLE_INFRA_DEPENDENCIES` and `ERR_INFRA_REQUEST_THROTTLING` error codes
The option to `Hide user issues` for operators has been replaced by an option to remove both user issues and temporary issues. This new filter is labelled as `Hide no operator action required issues`

Changed Hide user issues filter to hide no operator action required issues
@gardener-robot gardener-robot added needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jun 22, 2021
@grolu grolu requested a review from vpnachev June 22, 2021 15:37
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 22, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 22, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 22, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 22, 2021
Comment on lines 145 to 147
if (temporaryError) {
return 'mdi-clock-alert'
}
Copy link
Member

Choose a reason for hiding this comment

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

do you think we really need an own icon for temporary errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I thought has we highlight errors with known error codes, this could improve the ability to find out what is going on with your clusters faster. We can discuss this. As we also use this icon as an indication for cluster expiration it could irritate some users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a screenshot that shows the icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the dedicated icon

@vpnachev
Copy link
Member

Thanks, idea wise it looks good to me.
Code wise I am lacking the knowledge to provide valuable feedback.

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 23, 2021
Co-authored-by: Peter Sutter <peter.sutter@sap.com>
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 23, 2021
color="error"
:icon="userError ? 'mdi-account-alert' : 'mdi-alert'"
:icon="icon(userError, temporaryError)"
:prominent="!!userError ? true : false"
Copy link
Member

Choose a reason for hiding this comment

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

:prominent="!!userError"

Comment on lines 36 to 39
<div v-for="({ shortDescription, userError, temporaryError }) in tooltip.errorCodeObjects" :key="shortDescription">
<v-icon v-if="userError" class="mr-1" color="white" small>mdi-account-alert</v-icon>
<v-icon v-else-if="temporaryError" class="mr-1" color="white" small>mdi-clock-alert</v-icon>
<v-icon v-else class="mr-1" color="white" small>mdi-alert</v-icon>
Copy link
Member

Choose a reason for hiding this comment

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

Only the icon name is different. Why not a method like above?

Co-authored-by: Holger Koser <holger.koser@sap.com>
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 24, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 24, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 24, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 24, 2021
@@ -138,13 +138,10 @@ export default {
}
},
methods: {
icon (userError, temporaryError) {
icon ({ userError, temporaryError }) {
Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep this method remove the unused temporaryError

@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 24, 2021
Copy link
Member

@holgerkoser holgerkoser left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jun 24, 2021
Copy link
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 25, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 25, 2021
@grolu grolu merged commit 1ff5db0 into master Jun 25, 2021
@grolu grolu deleted the enh/new_error_code branch June 25, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/s Size of pull request is small (see gardener-robot robot/bots/size.py)
Projects
None yet
8 participants