fix(cli): clarify rule type deletion errors for bundles vs profiles#6310
fix(cli): clarify rule type deletion errors for bundles vs profiles#6310Aryanburnwal05 wants to merge 1 commit intomindersec:mainfrom
Conversation
|
|
Aryanburnwal05
force-pushed
the
fix/issue-5306-clear-delete-message
branch
4 times, most recently
from
e95a9d9 to
52fca0c
Compare
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
Thanks for testing your rule! I think we're likely to add something like #6286 in the future, but this will help keep things stable in the interim.
| if err == nil { | ||
| return false | ||
| } | ||
| return strings.Contains(strings.ToLower(err.Error()), "bundle") |
There was a problem hiding this comment.
What happens if there is a profile named "bundle" or "bundled-policies"?
There was a problem hiding this comment.
Good point thanks for calling this out. You are right that matching on a generic substring like "bundle" could lead to false positives in cases where profile names contain similar terms. Since the current server response doesn't expose structured error types for bundle constraints, I opted for a lightweight heuristic here as an interim solution. That said I can tighten this logic to reduce ambiguity (for example, matching more specific phrases), or adjust based on how error contracts evolve (e.g. #6286).
| cmd.Println() | ||
| cmd.Println("The following rule type(s) are referenced by existing profiles and were not deleted.") | ||
| for _, ruleType := range profileBlocked { | ||
| cmd.Println("- " + ruleType.Name) |
There was a problem hiding this comment.
This rewrite (diff) seems unnecessary -- can you revert it (and the similar ones below)?
In general, updating extra lines has a few negative effects:
- It makes code archaeology (e.g. via "blame" view) harder, because there are more changes to a given line, so it's harder to find the commit which actually changed behavior, rather than simply formatting.
- Human reviewers need to look at the changed lines to understand the proposed commit.
- A lot of extra lines make it more likely that humans will overlook critical changes due to inattention / fatigue
- Even without that, it simply adds time and effort for human reviewers to assess and dismiss the extra changes.
Aryanburnwal05
force-pushed
the
fix/issue-5306-clear-delete-message
branch
from
52fca0c to
7b4ed44
Compare
fix(cli): clarify rule type deletion errors for bundles vs profilesfix(cli): clarify rule type deletion errors for bundles vs profiles#
Aryanburnwal05
force-pushed
the
fix/issue-5306-clear-delete-message
branch
from
7b4ed44 to
4990cf6
Compare
|
Hi @evankanderson PTAL
|
| } | ||
|
|
||
| rules := []*minderv1.RuleType{ | ||
| {Id: "rule1", Name: "successful_rule"}, |
There was a problem hiding this comment.
I think Id needs to be a pointer to string
3 participants
This change improves the CLI output of
minder ruletype delete --allby clearly distinguishing between:It also improves readability by grouping results and refining CLI formatting.
Additionally, unit tests have been added to validate error categorization and parsing logic.