-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix: add empty check when no manifests are returned #26053
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: nitishfy <justnitish06@gmail.com>
❗ Preview Environment stop on Bunnyshell failedSee: Environment Details | Pipeline Logs Available commands (reply to this 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.
:lgtm: for thie small *fix
ppapapetrou76
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.
IMHO, this will create more confusion for the user...
If they read that the cache has issues, they will start to wonder what is wrong, etc.
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.
I also think user's will still run into the issue where they don't know whether their app actually has 0 manifests or the cache is down.
We would need to find a way to differentiate it these two errors
Without changing the server code it may be difficult, but one way is maybe we check the application status from the CLI?
Something like this
if len(resources.Items) == 0 && len(app.Status.Resources) > 0 {
// App has resources according to its status, but cache returned nothing
// This indicates cache is likely unavailable
errors.Fatalf(errors.ErrorCacheUnavailable,
"Application %s has %d resources but managed resources cache is unavailable. "+
appName, len(app.Status.Resources))
}
reggie-k
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.
I think that we need to have a clear distinguishing between the two types of errors, and the UI may need to have changes to deal with the error that would be returned from the server differently than it does today.
Closes #26038
After
Checklist: