perses
perses copied to clipboard
[ENHANCEMENT] Add --force option to allow applying resource despite project config inconsistencies
[ENHANCEMENT] Add --force option to allow applying resource despite project config inconsistencies
Description
Fix: https://github.com/perses/perses/issues/1917, take a look at the problem, i am uncertain if this solution aligns with our objectives. The proposed PR addition of a force flag aims to alert users to potential project inconsistencies while still permitting them to create based on input metadata.
Screenshots
Checklist
- [x] Pull request has a descriptive title and context useful to a reviewer.
- [x] Pull request title follows the
[<catalog_entry>] <commit message>
naming convention using one of the followingcatalog_entry
values:FEATURE
,ENHANCEMENT
,BUGFIX
,BREAKINGCHANGE
,DOC
,IGNORE
. - [x] All commits have DCO signoffs.
UI Changes
- na Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
- na Code follows the UI guidelines.
- na Visual tests are stable and unlikely to be flaky.
See Storybook
and e2e docs for more details. Common issues
include:
- Is the data inconsistent? You need to mock API requests.
- Does the time change? You need to use consistent time values or mock time utilities.
- Does it have loading states? You need to wait for loading to complete.
Also I think that an error message should be displayed at start of the command to prevent from applying half of the resources
Hello @ying-jeanne !
Thank you for taking the time to fix this issue :).
I'd see at a glance that we should reuse --all which is always used for "do this command for all projects" it would probably fit better the use case. @Nexucis WDYT ?
usage of --all
in the command apply
would be weird.
Reading percli apply -f data.json --all
, for me means I want to apply all data coming from the file data.json
. Which would mean we are partially applying the resources from the file.
From my perspective, introducing a flag like --force
is a nice way to solve it. If we don't like the meaning of --force
, we could have instead the flag --ignore.project-set
Also I think that an error message should be displayed at start of the command to prevent from applying half of the resources
That's something not possible currently as to be able to detect the inconsistency we need to loop other all resources that the user want to apply.
The only way to do that, is to loop a first time to search this issue.
I always see the force
flag as something that you shouldn't do but as an unusual emergency case. Here it can be a valid daily case. To just to not have to manage your different projects separately.
I'm wondering after all if a warning wouldn't be sufficient.
Note for the overthinking buddies like me: Also the warnings in general could be hideable. Like if we had a common system of warnings in this case saying "project-inconsistency: you applied in the project 'hello' which is different from current selected project 'moto'" and a common --ignore flag that you could use to ignore that specific warning --ignore project-inconsistency, that would be good End of note for the overthinking buddies like me...
Hi @celian-garcia @Nexucis both, thanks for the review . I would make it draft for the moment, since I realised later besides the --froce flag, It would be better to apply this consistency check in validate() function instead and introduce a multi-errors in order to go through all resources and return all inconsistencies in the response just as you described above. Will continue later this week and we can continue the discussion afterwards.
I usually lean towards --force option since it's my go-to when I'm confident about my actions after reviewing what just happened/warned. However, I'm also receptive to considering --ignore.project-set. Personally, I don't have very strong preference here. In the updated code I've included the multierr functionality and relocated the validation logic to the validate() method. If you would like to review it again, just let me know wdyt.
Wow crazy I've been able to resolve the conflicts from Github UI directly despite the fact your PR comes from your fork :astonished: Github will always surprise me. <3
So @ying-jeanne thank you very much! Let's move forward and merge it, your changes are very nice.
Wow crazy I've been able to resolve the conflicts from Github UI directly despite the fact your PR comes from your fork 😲 Github will always surprise me. <3
So @ying-jeanne thank you very much! Let's move forward and merge it, your changes are very nice.
I signed share my fork with project maintainer, that must be it! Back and force was so painful ❤️ thank you for the review! @celian-garcia
Looks better indeed. Thank you for your permanent work on this PR @ying-jeanne and sorry for the long delay it took to review / gives opinion.
Once last comment from @celian-garcia are tackled I am fine to merge it.
if by any chance you pass here to fix it soon @ying-jeanne , would be cool to merge it before making the new release !
comments addressed in commit 8d2f589 (: