Prompt: avoid resetting PR/issue metadata#2472
Conversation
mislav
left a comment
There was a problem hiding this comment.
Thank you for the fix; and good catch!
I noticed that there were not tests for this piece of logic. I do not fault you for not adding one, because it looks like making a test for this method is pretty hard. I will try to take over from this point. 🙇
pkg/cmd/pr/shared/survey.go
Outdated
| } | ||
| } | ||
|
|
||
| state.MetadataResult = nil |
There was a problem hiding this comment.
At first I wasn't clear why you did this, but then I tested it out and noticed that it fails in AddMetadataToIssueParams without this line.
I'm not convinced of this workaround, though, because it clears the cache of what was just fetched from the server and causes a whole bunch of records to be fetched again. I realize that fixing this properly is non-trivial, and I don't require that you add it to your PR. I'm currently exploring alternatives.
For metadata types chosen in interactive flow, we fetch all records from the API in order to be able to display a multi-select interface. For metadata defined via command-line flags, we resolve records that can be looked up directly, avoiding fetching the entirety of expensive datasets (e.g. all members of an organization) if we can. The new approach ensures efficient fetching when interactive flow is combined with values from flags.
mislav
left a comment
There was a problem hiding this comment.
I've pushed changes to resolve the metadata fetching conflict and to make MetadataSurvey easier to test, so we at least have some coverage around the interactive flow.
|
api/queries_repo.go |
Prompt: avoid resetting PR/issue metadata
Fixes #2369
Fixes #2475
The current implementation of
MetadataSurveyresets metadata provided by flags if no option is selected (pressEnterwithout selecting anything) and when the user adds metadata in the prompt.This happens because Survey doesn't return an error if you pass an empty slice of questions to it, so this check pass and
statemetadata fields are assigned tonilafter that.This PR contains the following changes:
mqsis empty before sending it to surveymqsisn't empty, fillvalueswith the values ofstate, that way it'll preserve metadata passed by flags.state.MetadataResultto nilThis is needed because
MetadataSurveysetsstate.MetadataResultto the selected options here, but if the user doesn't select anything or adds metadata thenAddMetadataToIssueParamsfails the get the IDs. By setting it tonilit'll be properly filled here.Demo: