When should issues be closed?

[jeffmendoza]

Currently the GitHub issue code is only triggered when:

• Allstar is enabled
• The policy is enabled
• The policy action is issue.

At that point. Allstar will ensure an issue is open if the policy fails, and ensure the issue is closed if the policy passes.

In any other case, Allstar does not check the issue status (open or closed). This means, Allstar will leave issues open if:

• The policy or Allstar is disabled
• The action is changed to log or fix.
• The issueRepo is set, unset, or changed.
• Allstar is uninstalled. (Can never close in this case).

Should Allstar try to close issues in some or all of these cases? If so, which ones? Allstar doesn't keep state, so this means it will search for issues in all those cases whenever a repo is checked.

[azeemshaikh38]

My 2 cents - let's say a policy is enabled with issue action and is seeing violations. The 3 possible updates the user could make:

1. Update the policy i.e disable or update some policy params (like add a binary artifact exception). In this case, we should ensure that the issue gets closed if the new policy params cause no violations (including the case where policy gets disabled).
2. Update the action type. Let's say the action type changed from issue to email, it would makes sense to me if the old issue did not get auto-closed. issue -> fix is a special case of this so I'm ok if the issue is left open in this case too. Another reason for me saying this is that, since AllStar doesn't keep state, we will need to add it only to support this which may not be worth the effort.
3. Update the issueRepo. Same as (2), we can leave the older issue open since we won't be spamming it anyways.

When we make architectural changes to AllStar backend for scaling, we can consider keeping state so that we have better support for (2) and (3).

@brianrussell2 fyi.

[agonzalez-plume]

Interestingly, I just updated Allstar branchprotection mode from issue -> log while diagnosing excessive Github emails going to some people (not all emails were from Allstar). I had wondered if that would close the Issue or not. I guess it doesn't, which is what I expected.

I do wonder if the priority for the Allstar check for a class of repos gets excluded or cancelled after initial configuration. If I optOut a class of repos, should that close the Issue? In that case, I believe it should close the issue.

[markdboyd]

@jeffmendoza I was just wondering about this myself. My opinion is that we should update Allstar to close issues if the action is set to "fix". Currently, you could have this experience:

• Action is set to "issue". Allstar runs scans, finds repos out of compliance, and creates Github issues.
• Then, action is set to "fix" to automatically fix the failing repos, but the Github issues remain open unless you set action back to "issue" and let the scans re-run

I can't really think of a downside to having issues closed if action is set to "fix" and I'm happy to implement this if you think it makes sense

[jeffmendoza]

Thanks for that fix @markdboyd ! We probably want to close issues in other cases as well, so leaving this open for more discussion.

[ethan7g]

In some of the cases listed it makes sense to close issues, so it might be best to do it if issueEnsure isn't explicitly called.

While there isn't much to do about the uninstalled case, I think a solution to all the other cases (except possibly issueRepo being changed? would have to look closer at how issueRepo is considered in enforce/issue) is to add a new feature to the issue package to be used in enforce runPolicies().

First, fetch all Allstar issues using the repo issues endpoint, setting creator to the app's username (if doing so works correctly), and then, when ensure is called, remove that issue from the list.

Once all ensureIssue calls are done, close issues remaining in list.

A struct to achieve that might look something vaguely like this and would exist in the issue package:

type IssueCurator struct {
    Issues []*github.Issue
}

func NewIssueCurator(ctx context.Context, c *github.Client, owner, repo string) error {
    iss, _, _ := c.Issues.ListByRepo(ctx, owner, repo, {Creator: test using Creator here with c.User.Get(ctx, "") ? })
    return &IssueCurator{iss}
}

func (i *IssueCurator) Ensure(ctx context.Context, c *github.Client, owner, repo, policy, text string) error {
    // ensure issue like usual
    // have ensure return whether an issue already existed, and its number if so
    // if already exist, remove from i.Issues the issue with that ID so it doesn't get closed
}

func (i *IssueCurator) Close(ctx context.Context, c *github.Client, owner, repo string) error {
    // close all remaining issues from i.Issues
}