There is no checklist here, only guidelines. There is no specific timeline on
this, but historically most backend trainee maintainers have become maintainers
five to seven months after starting their training.
As part of your journey towards becoming a maintainer, you may find it useful to:
Shadow a maintainer while they review an MR. This will allow you to get insight into the thought processes involved.
Have a maintainer shadow you while you review an MR as if you were a maintainer . Ideally, this would be with a different maintainer to the above, so you can get different insights.
You are free to discuss your progress with your manager or any
maintainer at any time. As in the list above, your manager should review
this issue with you roughly every six weeks; this is useful to track
your progress, and see if there are any changes you need to make to move
forward.
It is up to you to ensure that you are getting enough MRs to review, and of
varied types. All engineers are reviewers, so you should already be receiving
regular reviews from Reviewer Roulette. You could also seek out more reviews
from your team, or #backend Slack channels.
Your reviews should aim to cover maintainer responsibilities as well as reviewer
responsibilities. Your approval means you think it is ready to merge.
After each MR is merged or closed, add a discussion to this issue using this
template:
### (Merge request title): (Merge request URL)During review:- (List anything of note, or a quick summary. "I suggested/identified/noted...")Post-review:- (List anything of note, or a quick summary. "I missed..." or "Merged as-is")(Maintainer who reviewed this merge request) Please add feedback, and comparethis review to the average maintainer review.
Note: Do not include reviews of security MRs because review feedback might
reveal security issue details.
Tip: There are tools available to assist with this task.
FAQ
Should I ask for feedback for reviews on very small or trivial MRs that got merged without any additional feedback from maintainer?
Every trainee maintainer is encouraged to list all reviews, but may opt to not ask for maintainer feedback in trivial merge requests. This is also in sync with being respectful of other's time
Code Review Pairing
Much like pair programming, pairing on code review is a great way to knowledge share and collaborate on merge request. This is a great activity for trainee maintainers to participate with maintainers for learning their process of code review.
A private code review session (unrecorded) involves one primary reviewer, and a shadow. If more than one shadow wishes to observe a private session, please consider obtaining consent from the merge request author.
A public code review session involves a primary reviewer and one or more shadows in a recorded session that is released publicly, for example to GitLab Unfiltered.
If the merge request author is a GitLab team member, please consider obtaining consent from them.
If the merge request author is a community contributor, you must obtain consent from them.
Do not release reviews of security merge requests publicly.
When you're ready to make it official
When reviews have accumulated, you can confidently address the majority of the MR's assigned to you,
and recent reviews consistently fulfill maintainer responsibilities, then you can propose yourself as a new maintainer
for the relevant application.
Remember that even when you are a maintainer, you can still request help from other maintainers if you come across an MR
that you feel is too complex or requires a second opinion.
Create a merge request updating your team member entry using the template proposing yourself as a maintainer for the relevant application, assigned to your manager.
@huzaifaiftikhar1 "Creates audit event when approval rule is deleted" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mwoolf please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to remove redundant arguments from a method as we already had attribute reader defined for that instance variable.
I identified that incorrect object was passed to the base class. The base class expected an instance of type Project whereas we were passing an instance of ApprovalProjectRule.
Post review
Merged as-is
@mwoolf please add feedback, and compare this review to the average maintainer review.
Excellent review @huzaifaiftikhar1. You spotted a fairly serious issue in the MR, raised it in a constructive manner for the author to resolve and left me no work to do as a maintainer. 10/10.
@huzaifaiftikhar1 "Added read_usage_quotas ability to ProjectPolicy" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@toupeira please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested the specs to be added to the existing describe block instead of creating a new one as the changes were related to the the existing test.
I suggested to use subject to reduce duplicate line of code in the RSpecs.
I suggested the author to refactor the RSpecs to simplify the changes. The author didn't agree to it initially, however, when I pointed them to our documentation and the guidelines we follow, the suggested changes were applied.
I noted that we were missing RSpecs for admin and anonymous users and therefore asked to add Rspecs for it. Since the author was a new team member they couldn't understand the suggestion initially, however, when I explained the technical details they were able to understand and accepted the suggestion.
I identified that we had written code blocks that could have been removed if we used the existing helpers and shared contexts. This was again a bit difficult for the author to comprehend initially, however, later on they accepted the suggestion and also agreed that this was the best way to do it.
I noted that we were not following the commit message guidelines and added a suggestion to fix the commit messages to comply with the guidelines we have.
I missed the concept of methods that are defined dynamically in the Projects::ApplicationController This helped in removing the new method we defined and called in before_action.
@huzaifaiftikhar1 this was a great review, I appreciated your thorough comments and the MR was in very good shape when I came to it!
I missed the concept of methods that are defined dynamically in the Projects::ApplicationController This helped in removing the new method we defined and called in before_action.
Yeah this was easy to miss, and also not a very elegant part of our codebase
@huzaifaiftikhar1 "Stream audit event on merge request approval" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@brytannia please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to use a local variable instead of let_it_be as per our testing guide
I noted that the the value of target_details in audit events response was not consistent and therefore suggested to replace it with the MR title instead of MR id. This also helped the author unblock them to their original query of adding :iid.
I noted that the documentation update was missing after a change in the MR.
Post review
I missed suggesting the author to add a newline between variable declaration and test assertions.
@brytannia please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Remove block_project_serialization feature flag" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@engwan please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@proglottis please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested a couple of documentation fixes. The MR missed the 14.9 milestone so I proactively suggested the author to update the docs to reflect the correct milestone.
I suggested to make token_id as a required parameter.
I noted that sidekiq_inline was used in the RSpecs, however, it wasn't required so I suggested the author to remove it.
Post review
I missed one of the test case that was a false positive as it was using the top level variables defined instead of the override within the context.
@proglottis please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Migrate alert in _eoa_bronze_plan_banner to shared global alert" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@dskim_gitlab please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Migrate to alert advanced_search to shared global alert" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@vyaklushin please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Change verificationToken field in example" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@eread please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Check that possibleTypes for apollo are correct" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@vitallium please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
This was a completely new domain for me to review. I had never reviewed node CI scripts before. It was a good learning experience indeed.
I suggested to use isEqual method from lodash instead of writing our own isEqual method. This suggestion was discarded as we don't need to yarn install in the CI and thus save some time / bandwidth!
I identified that the change doesn't add a newline at the end of the file app/assets/javascripts/graphql_shared/possible_types.json. I suggested a code change to resolve this.
Post review
A suggestion was made by the frontend maintainer to remove the isEqual method and use assert.deepStrictEqual instead.
Another suggestion was made to replace process.exit(1) with process.exitCode = 1 so that node process doesn't gets killed abruptly.
@vitallium please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Refactor titles, add version text, and other refinements" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@msedlakjakubowski please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Limit audit events controller to 31 days date range" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@stanhu please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to inline the new before_action since the other conditions for triggering the new action was same as the previous one.
I identified that we had duplicate context blocks across three different files and therefore suggested to create a separate shared context file and then include it in all the three.
I identified that validate_date_range method will throw exceptions in case we pass date as an empty string. I suggested the author to use present? to prevent this.
Post review
Merged as-is
@stanhu please add feedback, and compare this review to the average maintainer review.
Good review. I was slightly inclined to push the author to handle the error cases you mentioned, but since this was only on the admin page I decided it was okay.
@huzaifaiftikhar1 "Graceful degradation for refs endpoint" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@marc_shaw please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
We were returning the error message in the response which could potentially reveal unintended information. I missed suggesting that we should return a generic message instead.
@marc_shaw please add feedback, and compare this review to the average maintainer review.
I don't think this would have been a problem to return the message, but in a complex system I am not 100% of this, and don't think the risk/reward is worth it here.
@huzaifaiftikhar1 "Add GraphQL option to mark all todos of a specific target as done" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@reprazent please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Fix compliance violations pagination so it paginates all sort options properly" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@ebaque please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Change import project members API feature category" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@DylanGriffith please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Remove geo_token_user_authentication feature flag" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mkaeppler please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I identified that the sub-category for the type feature label was incorrect as per the work type classification and hence updated it and informed the author about the same.
Post review
Merged as-is
@mkaeppler please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Move compliance framework auditor to a new class" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@eurie please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to use scope resolution operator at the global namespace to avoid breaking the code in case we refactor and move the files to a different directory.
I suggested to rename the new class from ComplianceFrameworkAuditor to ComplianceFrameworkChangesAuditor so that it that it is consistent with other classes.
I suggested the variable name to be renamed (from model to compliance_framework_setting) so that it's self explanatory.
I suggested to inline merging two hashes.
Post review
I missed suggesting to add rspecs for one of the cases.
@eurie please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Use arel order for "order_id_desc" scope" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@reprazent please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mwoolf please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to use DateTime.current instead of Time.zone.now to be consistent with the existing code and avoid any unforeseen issues.
I identified that an update to the development guideline documentation was missing and therefore suggested to update it as well.
I suggested to not explicitly convert created_at key to time while logging to the audit_json.log file as this is handled automatically by the logger itself. I also verified the same using rails console and later on the author agreed to my suggestion.
I suggested to clean the code a bit by not calling .merge multiple times on the same hash object and instead use the existing base_payload method where the default hash could be declared.
I noted an undesired change (typo) and asked the author to revert it.
I identified that a review from the technical writer was recommended and therefore requested for the same pro-actively.
Post review
I missed the fact that there were places in the Rspecs that used anything instead of asserting the specific values.
Some minor nits (RSpec example rename)
@cablett please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 I thought your review was nice and thorough, great work
I think it's important to note that the title of the MR was slightly different to what the code actually did (old title was "ability to pre/post date audit events"). Ensuring the MR title and description are accurate and well explained is super important for future viewers to immediately understand exactly what change the MR represents
Ensuring the MR title and description are accurate and well explained is super important for future viewers to immediately understand exactly what change the MR represents
Definitely, I agree. Thanks for the feedback @cablett. I will take care going forward.
@huzaifaiftikhar1 "Add audit events for merge request settings" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@engwan please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to rename a few variables to make them self-explanatory. Example from foo_instance to project_ci_cd_setting_changes_auditor.
I identified a potential issue where multiple audit events were recorded the first time a merge request setting was updated.
Post review
A follow-up issue was created to fix the incorrect usage of EE module. Since this was an issue before this MR therefore the maintainer created a separate issue to fix this.
@huzaifaiftikhar1 "Remove unused MD5 generation logic for InsecureKey" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@pshutsin please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Merge branch '301124-merge-request-setting-audit-events' into 'master'" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mwoolf please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Add validation for namespace on compliance frameworks" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@eurie please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@nmilojevic1 please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
This was a quite a lengthy MR and I checked all the files that were removed and ensured that they are not being referred to in the code anymore. (Used grep and also relied on RubyMine to find the usage).
I suggested to remove the prepend_mod_with from the CE model as we had removed the EE overload.
Post review
Merged as-is
@nmilojevic1 please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Swap EscalationPolicies::BaseService to use ::BaseProjectService" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@smcgivern please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
Replace generic checkbox with GitLab UI styled checkbox in app/views/devise/shared/_email_opted_in.html.haml: gitlab-org/gitlab!84858 (merged)
@huzaifaiftikhar1 "Replace generic checkbox with GitLab UI styled checkbox in app/views/devise/shared/_email_opted_in.html.haml" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mrincon please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "PipelinesUsageApp: Buy Additional Minutes button" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mayra-cabrera please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@pshutsin please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Fix impersonation created_at audit event field" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@alipniagov please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@ebaque please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Add audit events for merge request settings" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@alipniagov please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to use human friendly name instead of model.class.name,
I identified that the audit event message was a bit misleading and therefore suggested to update it.
I noted that we started memoizing a variable and therefore suggested to run package-and-qa as a precaution.
I suggested to create a base service to avoid code duplication.
I noticed incorrect usage of the EE module and suggested to move the class out of the EE module.
I suggested to be more descriptive in the RSpecs and also suggested to add assertions on the changed values.
I noticed that we were creating M:N association in the RSpecs, however, on the UI we only supported 1:1 mapping, so I asked for the reason why we had it like that. The same question was also asked by the maintainer later on.
I noticed that we were declaring redundant variables that were not used every time, so I suggested to move them inside the block they were being for optimisation.
Post review
I missed suggesting to use empty? instead of blank? on active records associations.
I missed suggesting to move the human friendly naming logic to a method.
I missed suggesting to skip the method argument since the argument was an instance variable and we didn't require to pass it as an argument.
I missed suggesting to make the audit event message when updating squash options to a more descriptive message.
@alipniagov please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 You did a great review there, thanks!
I received an MR in a good shape.
Yes, I noticed some things that were not covered by the initial review, but it's OK.
Special thanks for keeping an eye on the discussion during the maintainer review, I appreciate it.
My suggestions would be:
Keep an eye on a typical performance (anti-)patterns, we have surprisingly many places where we could improve. And usually, it's related to the object allocation - pulling too much data or not paginating, etc.
Overall, the MR was pretty huge compared to the typical MR I usually see, we may consider splitting such MRs if possible (although I didn't see a clear boundary in our case).
Thanks @alipniagov. I usually keep an eye on the performance issues but I will make sure pay more attention to it going forward. Thanks a lot for the suggestions I really appreciate your time on this.
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@godfat-gitlab please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Nullify merge_request_metrics pipeline_id on pipeline deletion" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@ebaque please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
Resolve "Update batched background migration information to admin panel with a migration detail page": gitlab-org/gitlab!84929 (merged)
@huzaifaiftikhar1 "Resolve "Update batched background migration information to admin panel with a migration detail page"" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mkozono please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Remove ability for SSH key expiration to be optional" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@terrichu please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I noted that we remove the "Key becomes invalid on this date" text from the UI which was still needed.
I suggested to not modify lib/gitlab/git_access.rb and add the expiry validation there, instead suggested to move the validation to the model itself.
I suggested instead of completely removing the expiration validation I suggested to keep it and move it to the CE version.
I noticed we removed gl_console_messages and since I was not sure about its impact I suggested the author to get those changes reviewed by the domain expert. I also notified the maintainer about the same while passing the MR over to them.
I suggested to update the changelog to reflect the correct status i.e. removed.
I suggested to remove the duplicate code from the EE version and use super instead.
I noticed an unintended change and suggested the author to revert it.
I made a non-blocking suggestion to revert the changes of moving the method to a different location in the same file so that unnecessary changes are not seen in the diff.
Instead of removing an RSpec completely I suggested to update it to return the new return string of the modified method.
I suggested to skip the validations while creating the factory objects instead of adding a new before block everywhere.
Post review
I missed suggesting to add a new trait to the factory instead skipping the validations while creating the factory objects inside an RSpec.
I missed suggesting to only do the expired validations when the key is created, otherwise, an already valid model could become invalid after a certain time.
@terrichu please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Resolve "Ensure batched background migrations admin interface is multi-database aware"" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@DylanGriffith please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
@huzaifaiftikhar1 "Remove repository push audit event feature" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@jprovaznik please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I identified that the MR will break a documentation URL and therefore asked the author to fix it. This was missed during the documentation review.
I noted the change removed a feature so I added the pipeline:run-all-rspec label as a precaution and triggered a new pipeline.
Post review
Merged as-is
The maintainer noticed that we might have to consider backward compatibility while removing the sidekiq worker, however, they moved forward with merging the MR since it felt like it was not needed. Later on I realised that the backward compatibility issue raised was a genuine concern and suggested to fix it in a follow-up.
@jprovaznik please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Do not allow expired personal access tokens to work" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mwoolf please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to remove the unused method show_token_expiry_notification? which was not used anymore.
I identified that the changelog didn't follow our changelog entries guideline and therefore asked the author to update changelog from changed to removed.
I noted that the case when_active_or_expired inside personal_access_tokens_finder.rb was not used anymore so I suggested to remove the dead code.
Since I wasn't sure about why we needed to update the GraphQL N+1 RSpecs in this MR, therefore I asked the author about the same. Since we were not too sure whether it was correct or not we decided to choose a backend maintainer with GraphQL expertise.
I noticed that we needed a database review as well so out of bias for action I requested for a database review.
Post review
I missed seeing an empty method def current_settings which was not related to this MR but the maintainer suggested to remove it if it wasn't required.
I missed one the strings in the RSpecs that had to be updated.
Of course I missed the Marmite vs Vegemitesuggestion
I missed suggesting to add an expired token to the list of tokens and ensure that it doesn't appear in the table.
A lot of folks were involved resolving the GraphQL N+1 RSpec and later on we realised that it wasn't the changes in the MR that was the root cause, however, the MR just helped us discover the issue. The maintainer created an issue for the same. The maintainer also created an MR to ignore the License SQL calls in query recorders. We finally reverted all the GraphQL N+1 RSpec changes from this MR.
@cablett please add feedback, and compare this review to the average maintainer review.
I thought your review was great @huzaifaiftikhar1 ! Your devopsmanage expertise was clear in your review. Thanks for checking with a Database and GraphQL reviewer, always a good idea to err on the side of caution since there are gotchas with those technologies.
Whenever there is a process being followed (in this case ignoring a column) I like to double-check the milestones and ensure there's an issue to do any future work (gitlab-org/gitlab!85399 (comment 927539036))
Thanks for the feedback @cablett. Yes I was already aware of the follow-up issue for removing the column, I should have probably asked the author to add it to the MR description for transparency and it would have helped in avoiding you to ask that question.
@huzaifaiftikhar1 "Add repository push audit event worker" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@jprovaznik please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I suggested to add a comment explaining why the perform method of the sidekiq worker is empty.
I created a follow-up issue to remove the sidekiq worker completely.
I noted some changes that were not relevant to the MR and asked the author to revert them. The author later on acknowledged that it was pushed by accident.
Post review
Merged as-is
@jprovaznik please add feedback, and compare this review to the average maintainer review.
@huzaifaiftikhar1 "Replace approved with passed in status check api" was merged
convert this into a discussion using the following template
## During review- I suggested ...- I identified ...- I noted ...## Post review- I missed ...- Merged as-is@mwoolf please add feedback, and compare this review to the average maintainer review.
This message was generated automatically using Review
Tanuki. Do not edit manually
I noticed that we were not removing the feature flag entirely and ensured that we will do the same in another issue so that the breaking change is released in 15.0 itself.