-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: show warning if policy doesn't exist #16437
feat: show warning if policy doesn't exist #16437
Conversation
a8640ee
to
74343cf
Compare
Hey @farbodahm! Just a heads up that we're having a fairly intense (but friendly! 😀 ) internal debate about how we actually want to handle this whole missing-policy issue, because it has a lot of backwards compatibility issues as discussed in #14058. We'll probably at least want to have this warning, so thanks for that. We'll try to get back to you on this soon. |
@tgross Hey man. Thank you for the update. |
Hey @farbodahm, I apologize for not responding to you sooner. I was reviewing your request and I had what I thought at the time would be a simple organizational ask about consolidating the policies that weren't found into a single warning line. In looking more deeply, I realized that, other than string-scraping, there is currently not a way to know if an unexpected response error was because of a not found or because of a server error. To address this issue, I started working on #16743, which can then be used by code to more concretely validate the error reasons and provide more specific warning text in these cases. I am going to assign this PR to myself and will follow up with you once 16743's fate is decided. |
74343cf
to
5992d90
Compare
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.
Looking to do a little bit of PR and issue cleanup and found this PR. While it'd be great to rework this and other bits of the CLI to take advantage of the shapely error work done in #16743, better to just go ahead and land this if we don't have the time to follow-up. I've rebased this PR on main and pushed a commit with a changelog entry.
Will merge once green and then this will ship in Nomad 1.8.3
This pr is related to #14058 .
I wanted to work on that issue, but as I saw still it's not quite obvious on how we are going to fix it, I decided to only show a warning so at-least user can be aware the policy he/she is referring to, doesn't exist at the moment.
Also I can continue fixing this one if the expected result is known now.
Thanks.
Fixes: #14058