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
pr checks: avoid deduplicating same-named checks under different workflows #5919
Conversation
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.
Looks good!
Since workflow name is an important piece of checks information that we previously omitted from gh pr checks output, should we consider prepending the workflow name to checks name, like the web interface already does? This could be either in this PR or as follow-up.
I'm in favor of this. It's up to you if you want to do it now or in a follow-up.
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.
Overall the code looks good. I do think we should prepending the workflow name to checks name, but can be done in a follow up PR. My only concern here is the addition of the gojsondiff
package.
api/export_pr_test.go
Outdated
@@ -9,6 +9,8 @@ import ( | |||
"github.com/MakeNowJust/heredoc" | |||
"github.com/stretchr/testify/assert" | |||
"github.com/stretchr/testify/require" | |||
diff "github.com/yudai/gojsondiff" |
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.
It seems a bit heavy handed to require a new library just for a single test... I am wondering if there is an easier way to make the same assertions without requiring adding a whole new library to the project? Additionally, the library doesn't seem to be actively maintained which is a bit concerning.
Our checks de-duplication logic was a bit too overzealous in a way that it would squash same-named checks coming from different workflows under the same entry in
gh pr checks
output.Bonus: this also adds
workflowName
for Checks andstartedAt
for Statuses ingh pr view --json statusCheckRollup
output.Fixes #5898
TODO:
gh pr checks
output, should we consider prepending the workflow name to checks name, like the web interface already does? This could be either in this PR or as follow-up.