-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Prompt: avoid resetting PR/issue metadata #2472
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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.
api/queries_repo.go
api/queries_repo.go |
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.
ios
Prompt: avoid resetting PR/issue metadata
Fixes #2369
Fixes #2475
The current implementation of
MetadataSurvey
resets metadata provided by flags if no option is selected (pressEnter
without 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
state
metadata fields are assigned tonil
after that.This PR contains the following changes:
mqs
is empty before sending it to surveymqs
isn't empty, fillvalues
with the values ofstate
, that way it'll preserve metadata passed by flags.state.MetadataResult
to nilThis is needed because
MetadataSurvey
setsstate.MetadataResult
to the selected options here, but if the user doesn't select anything or adds metadata thenAddMetadataToIssueParams
fails the get the IDs. By setting it tonil
it'll be properly filled here.Demo: