Skip to content
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

feat: add [gitlabmergerequests] service #8166

Merged
merged 8 commits into from
Aug 17, 2022

Conversation

sunny0826
Copy link
Contributor

re #8077

@shields-ci
Copy link

shields-ci commented Jul 5, 2022

Warnings
⚠️

📚 Remember to ensure any changes to config.private in services/gitlab/gitlab-merge-requests.service.js are reflected in the server secrets documentation

Messages
📖 ✨ Thanks for your contribution to Shields, @sunny0826!

Generated by 🚫 dangerJS against 309df89

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jul 5, 2022
@calebcartwright
Copy link
Member

Have only done a quick pass through this but a couple early notes:

  • given the 10k ceiling, that seems like a case we should handle explicitly in our rendering, even if it requires a special case handling
  • consider if/how the handle/fetch/transform function paradigm could be applied with this class

@calebcartwright
Copy link
Member

@sunny0826 just want to make sure you saw this 👉 #8166 (comment)

@sunny0826
Copy link
Contributor Author

@sunny0826 just want to make sure you saw this 👉 #8166 (comment)

I saw this, but wasn't sure what I needed to do.

@calebcartwright
Copy link
Member

@sunny0826 just want to make sure you saw this point_right #8166 (comment)

I saw this, but wasn't sure what I needed to do.

Ah okay, no worries! In the future don't hesitate to ask questions in these types of cases, especially if something is unclear. I'll follow up tomorrow with some elaboration on my prior comment so we can figure out next steps

@shields-cd shields-cd temporarily deployed to shields-feat-gitlab-mr-pumlwrn July 22, 2022 03:08 Inactive
@calebcartwright
Copy link
Member

After looking through a little more closely, I see that you are already applying special case handling to the >10k scenario which is good 👍 (I was just emphasizing that we should in my prior comment).

The second part of my comment was really just related to the length of some of those methods, and a suggestion to consider using the transform function to encapsulate some concerns and hopefully shorten some other functions. However, I'm not sure how much that will really buy given the nature of what's going on in those two other functions. I'll try to leave some inline feedback over the weekend as I think there's some other changes that can make those a little shorter, and we can reconsider if a separate transform function is still worthwhile afterwards.

Finally, thanks again for all your work on these GitLab badges, it's very much appreciated!

@sunny0826
Copy link
Contributor Author

After looking through a little more closely, I see that you are already applying special case handling to the >10k scenario which is good 👍 (I was just emphasizing that we should in my prior comment).

The second part of my comment was really just related to the length of some of those methods, and a suggestion to consider using the transform function to encapsulate some concerns and hopefully shorten some other functions. However, I'm not sure how much that will really buy given the nature of what's going on in those two other functions. I'll try to leave some inline feedback over the weekend as I think there's some other changes that can make those a little shorter, and we can reconsider if a separate transform function is still worthwhile afterwards.

Finally, thanks again for all your work on these GitLab badges, it's very much appreciated!

Thank you for your feedback.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks and apologies for the delay! I think the core of this is in place, just have a few suggestions/requested changes detailed below

services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.tester.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.tester.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.tester.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.tester.js Outdated Show resolved Hide resolved
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thank you for the updates. Few additional asks but think we're on the home stretch.

These issue/PR/MR types of badges can get quite involved so thanks for sticking with it!

services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Show resolved Hide resolved
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates and apologies for the delay in review. Few final items then think we'll be set

services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Outdated Show resolved Hide resolved
services/gitlab/gitlab-merge-requests.spec.js Outdated Show resolved Hide resolved
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed one final thing, noted inline below

services/gitlab/gitlab-merge-requests.service.js Outdated Show resolved Hide resolved
@repo-ranger repo-ranger bot merged commit e95189c into badges:master Aug 17, 2022
@sunny0826 sunny0826 deleted the feat/gitlab-mr branch August 17, 2022 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants