Fix CommitSigningConfiguration resolver err#52965
Conversation
courier-new
left a comment
There was a problem hiding this comment.
Thank youuuu! This is a huge help.
| return nil, ErrNoGitHubAppFound{Criteria: strWhere} | ||
| swhere := where.Query(sqlf.PostgresBindVar) | ||
| for i, arg := range where.Args() { | ||
| swhere = strings.Replace(swhere, fmt.Sprintf("$%d", i+1), fmt.Sprintf("%v", arg), 1) |
There was a problem hiding this comment.
What are you replacing here exactly @courier-new ?
There was a problem hiding this comment.
Apparently swhere := where.Query(sqlf.PostgresBindVar) only binds the variables, but not their values. So if you logged the error message, it'd look like:
no app exists matching criteria: 'domain = $1 AND base_url = $2'
which is helpful but obviously still not quite what we were going for. 😅 I couldn't seem to find a sqlf.Query method that did this automatically, since normally it happens at DB Query time based on the DB context, but I did find a handful of other places in the code where we build the full string with values by iterating over the sqlf.Query.Args() and strings.Replace()-ing them at their indices in the original string, so that's what I decided to do here, too. 🙂
There was a problem hiding this comment.
I see, I see. Thanks for catching this! 🙏🏻
Fixes github_apps store resolver to not error if an app is not found. Adds
ErrNoGitHubAppFoundtype assertion to create a generic error type to re-use for any github_app that can't be foundPartial pair with @courier-new the great 👑
Before
After
Test plan
/site-admin/batch-changesroute doesn't error on initial state (when you haven't configured any github_apps yet)