-
Notifications
You must be signed in to change notification settings - Fork 993
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
Fix type definition for approvers in pull request creator #8793
Conversation
@@ -150,7 +150,7 @@ def add_approvers_to_merge_request(merge_request) | |||
end | |||
|
|||
def approvers_hash | |||
@approvers_hash ||= approvers&.transform_keys(&:to_sym) || {} |
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.
Since technically this regression is already released, might make sense to just force using specific type and not have to perform this conversion.
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.
Thanks @andrcuns ; I assume this regression impacts GitLab's interfaces specifically (where the approvers can be a hash versus array of strings?)
Is this accurate?
Also tagging @JamieMagee , @deivid-rodriguez , and @jakecoffman
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.
@andrcuns could you share the error message you got?
@abdulapopoola it is likely correct, but we don't test dependabot-core against GitLab so it's hard for us to see.
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.
@JamieMagee Yes, this is specifically for the Gitlab's interface. Maybe other implementations could be impacted as well, but I did not go through other vendors. The error would look something like this when trying to set gitlab specific reviewers/approvers.
Parameter 'reviewers': Expected type T.nilable(T::Array[String]), got type Hash with value {:approvers=>[3], :reviewers=>[2]}
Caller: /home/dependabot/app/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11190/lib/types/private/methods/call_validation.rb:217
Definition: /home/dependabot/app/vendor/bundle/ruby/3.1.0/gems/dependabot-common-0.240.0/lib/dependabot/pull_request_creator.rb:174
This is because in GitLab, you can set approvers
, reviewers
and approver_groups
.
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.
Thanks! Yeah, this looks good for now.
In the long run, I would like to refactor this so we're not passing around platform-specific responses to the platform-agnostic PullRequestCreator
. we likely need some common interface that all platform-specific responses get translated to, with a platformSpecificMetadata
property. Or something like that.
Current type for the main PullRequestCreator interface prevents adding reviewers for Gitlab configuration because it is a bit more elaborate and accepts hash with different types of reviewers.