Refactor List-Rules #381
Refactor List-Rules #381
Conversation
…y-staging Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
| for _, key := range curRule.Delegation.Role.KeyIDs { | ||
| fmt.Printf(strings.Repeat(" ", curRule.Depth+2)+"%s\n", key) | ||
| } | ||
| fmt.Print(policy.GetDiffBetweenPolicyAndStaging(policyRules, policyStagingRules, policyRoot, policyStagingRoot)) |
There was a problem hiding this comment.
This would be a good candidate to route to the display package when #282 lands.
There was a problem hiding this comment.
I was also thinking the same thing. I don't think it really matters which PR lands first, but #282 definitely needs some attention, since without it, it is quite hard to know what a mantainer is approving when they create a reference authorization.
| @@ -21,6 +21,7 @@ import ( | |||
| "github.com/go-git/go-git/v5/plumbing/filemode" | |||
| "github.com/go-git/go-git/v5/plumbing/object" | |||
| sslibdsse "github.com/secure-systems-lab/go-securesystemslib/dsse" | |||
| "github.com/sergi/go-diff/diffmatchpatch" | |||
There was a problem hiding this comment.
I'm unsure if we want to introduce another dependency. It is however only used for diffing text to be printed out for the user and to my knowledge the original library from Google is no longer maintained, so it might be ok.
There was a problem hiding this comment.
Thanks, I don't know what else I could use, if you have any other suggestions for what we could use, I am open to ideas.
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
| func getRuleString(rule tuf.Delegation, depth int) string { | ||
| var changes string | ||
|
|
||
| changes += fmt.Sprintf(strings.Repeat(" ", depth)+"Rule %s:\n", rule.Name) |
There was a problem hiding this comment.
Could this be cleaned up a bit so there's not a mix of + to concat strings and fmt.Sprintf?
There was a problem hiding this comment.
Wonder if the indent string at one scope can be computed once too 🤔
| @@ -818,7 +819,122 @@ func ListRules(ctx context.Context, repo *git.Repository, targetRef string) ([]* | |||
| } | |||
| } | |||
|
|
|||
| return allDelegations, nil | |||
| rootMetadata, err := state.GetRootMetadata() | |||
There was a problem hiding this comment.
ListRules returning the root metadata is a bit counterintuitive, no? I wonder if the consumer for this can load the root metadata separately, so this func is tighter.
| for _, key := range curRule.Delegation.Role.KeyIDs { | ||
| fmt.Printf(strings.Repeat(" ", curRule.Depth+2)+"%s\n", key) | ||
| } | ||
| fmt.Print(policy.GetDiffBetweenPolicyAndStaging(policyRules, policyStagingRules, policyRoot, policyStagingRoot)) |
There was a problem hiding this comment.
This is a bit of a higher level thought but rolling list-rules and diff into one is counter intuitive to me. Instead, diff should be its own utility that a user can pass policy/staging RSL entries too. That way, they can also see the diff between any arbitrary policy states, no?
If we want to do a separate diff subcommand and show a diff here when staging exists on top of policy, some additional logic may be in order to verify staging is ahead of policy and so on.
|
@neilnaveen Can you rebase/rework this as the |
|
Hey @neilnaveen, are you still working on this? |
Fixes #371 (comment)