Skip to content

Add Jiaan Louw as frontend maintainer of GitLab

Jiaan Louw requested to merge jiaan-master-patch-55200 into master

Self-assessment

After >2 years at Gitlab and almost 6 months as a trainee maintainer I want to propose myself as ~Frontend maintainer. In this merge request description I have listed for you my justifications and the relevant and merge requests that I have reviewed and authored. Additional reviews can be found in my trainee maintainer issue.

  • Advanced understanding of the GitLab project as per the documentation:
    • I have authored and reviewed merge requests that cover multiple technologies used in the GitLab codebase, this includes Vue, Vuex, GraphQL/Apollo, HAML, Jest, SCSS, HTML best practices and I'm also comfortable working with the GitLab backend and APIs. Additionally I have authored and reviewed a wide range of changes that touch multiple areas and tools in the GitLab codebase, such as merge request, projecs, group and admin-level tools and reporting. I have also taken on large migrations such as converting the admin/users page from HAML to Vue. Lastly I have successfully advocated for adding a local setup and validation section to the default GitLab project MR template, since that's been introduced I've observed an increase in the number of MRs I review with setup instructions [1, 2, 3] and aided me in validating these changes.
  • The MRs reviewed by the candidate consistently make it through maintainer review without significant additionally required changes.
    • Most of my merge request reviews have made it through maintainer review with little to no additional feedback and have been merged as-is. I am mindful creating feedback that is blocking and I am in favour of creating follow-ups when a suggested change is outside the scope of the original change to keep up the author's momentum. Additionally I always try to locally validate any MR and note that down for other reviewers so that a maintainer knows that a change has been verified.
  • The MRs authored by the candidate consistently make it through reviewer and maintainer review without significant required changes.
    • The majority of my merge requests get to be merged with minor required changes. I am open to any recommendations and questions, and when I am unsure of or disagree with a recommendation I try to communicate that openly, look for precedent and ask for additional feedback from another reviewer when possible.

Links to Non-Trival MRs I've Reviewed

MR Title Changes Link Comments
Allow groups to be re-imported 497-159 gitlab-org/gitlab!68055 (merged) This was a fairly large and non-trivial change by groupimport on a tool that I'm not familiar with. My review consisted mostly of suggestions on the code, like improving the readability and increasing our spec coverage, and a few suggestions for UX improvements, like using a pointer and improving row alignment. Even though I had some initial trouble in getting the change running on my GDK, I wanted to be sure that I could validate the changes locally and I think this paid dividends because it enabled me to make the UX suggestions. I tried to be mindful of when my comments where out-of-scope because this was such a large MR already, and I was supportive of creating follow-up issues. Maintainer feedback.
Add import members from project modal 730-18 gitlab-org/gitlab!66634 (merged) This was another large change on a page where I lacked domain knowledge. My review consisted of various comments, but I mostly focussed on checking for bugs, improving the tests, and I provided patches for larger suggestions. The was a fairly lengthly review that lasted 3 or 4 rounds, but I think we ended up with a change that was much better than it started out as. Maintainer feedback
Add the drawer to the compliance report application 343-72 gitlab-org/gitlab!76431 (merged) This was a relatively non-trivial change that involded rewriting the existing compliance dashboard elements in preperation for migration to the new compliance report, however as a member of groupcompliance this is an area where I have ample domain knowledge. The majority of my review was focussed on code improvements. During the review I identified where we could improve efficiency by map data on apollo update instead of on every toggle event, and I also suggested how we could improve the drawer component properties to make it easier to implement for developers. Maintainer feedback.
Create a new merge request violations compliance report application 621-30 gitlab-org/gitlab!75015 (merged) This was a non-trivial change, however since it was in my group groupcompliance I had ample domain knowledge. For this change I did an early review to verify the implementation and provide high-level feedback. I then followed up with a full review during which I made suggestions to trim unused attributes from our GraphQL query and improve the readability of the tests by using it.each. Maintainer feedback.
Prevent design discussions without user login 372-45 gitlab-org/gitlab!77563 (merged) This was a relatively large change to the design discussion section, a domain I'm not very familiar with outside of using it myself. During review I identified where we could simplify the template to make it more readable, update strings to comply the latest Pajamas recommendations, update the string placeholders to be consistent with the rest of the codebase, and recommended we use GlSprintf instead of sprintf because it was being used to interpolate a link. After my review the change was merged as-is without any further maintainer comments.

Smaller Reviews with No Maintainer Additions

MR Title Changes Link Comments
Add content validation service entry in admin setting page of JH edition 3-0 gitlab-org/gitlab!73166 (merged) This was a very small review, however I was unsure why this change was needed and discovered that we didn't have good explanation or guidelines on how to review JiHu additions to GitLab.com, so I followed up with a MR to improve our documentation and noted that down for the frontend maintainer. I also suggested we add a comment so that the JiHu addition doesn't accidentally get removed because it's referencing a file that doesn't exist in the GitLab codebase.
Improve deteriminism of simulateDrag 30-12 gitlab-org/gitlab!75303 (merged) This MR changed a fairly odd piece of code used in our capybara specs. During review I made several suggestions on how we could make the function more readable and I also asked if we could move the frontend tests out of capybara for which a follow-up was created.
Add basic components to display dep proxy manifests 292-19 gitlab-org/gitlab!72869 (merged) During this review for ~"group::package" I noticed a small visual bug / inconsistency and asked if we could replace custom components with ones from GitLab UI. I also added some praise for the excellent setup & testing instructions. Maintainer feedback.
Integrate instance-level with group-level MR approval settings 332-26 gitlab-org/gitlab!67316 (merged) This was a fullstack merge request where the only frontend change was a change to the data structure. During the review I identified where we could improve the consistency of our API attribute names, for which I created a follow-up issue which was then resolved, and provided a patch to help resolve the failing frontend tests.
Migrate epic sidebar todo button to widget 43-39 gitlab-org/gitlab!68325 (merged) During review the only feedback I had was on how we could improve the tests. I identified one scenario that was essentially a duplicate and could be removed, and another scenario that we could add that wasn't covered.
Resolve "GitLab Migration - More fine-grained status for group bulk imports" 176-96 gitlab-org/gitlab!73960 (merged) This change updated the status reporting on the group import workflow. During review I suggested that we could improve the UX by providing more space for the status message and also asked if we could distinguish the status updates from the usual input validation messages. Finally I added praise for how easy it was to locally validate the change due to the well written testing & setup instructions.
Add a feature for setting the maximum allowable lifetime for SSH keys at an instance level 294-25 gitlab-org/gitlab!75098 (merged) This was another full stack merge request with minimal frontend changes. During review I left a suggestion where we could improve the form UX by limiting the date range instead of providing input validation with an alert on form submission. Even though I only had one suggestion I think that the UI was in a much better state than it was before.
Use GlAvatar in approval rule selector 62-35 gitlab-org/gitlab!73334 (merged) This was a really good change and during review I only had minor suggestions to improve the readability of the Vue property validator and create a shared constant for common strings.
Removes unnecessary tabs from DAST view scans 88-199 gitlab-org/gitlab!72875 (merged) During review I suggested we improve the spec coverage and noted that a test that was being removed should be kept partially. I also had high praise for how well structured and written the commits were.
The syntax highlighting doesn't work when creating a new file from a template 58-1 gitlab-org/gitlab!71801 (merged) This was a relatively minor change and I only had two minor comments to improve the spec coverage and to update the changelog entry to better match the type of change.

Links to Non-Trivial MRs I've Written

MR Title Changes Link Comments
Add merge request violation component for compliance reports 248-44 gitlab-org/gitlab!76214 (merged) This MR added a component to render the merge request violation. It was part of a series of MRs and issues to upgrade the compliance report. It involved using a object to match the compliance violation code number to a human readable reason. I also took the opportunity to update the files I was working on to use a new shared UserAvatar component. Because there are multiple files changed for multiple reasons I decided to break up my changes into separate smaller commits and added inline note comments to make the change easier to understand and thus review.
[Frontend] Update audit events filter bar to search by username 992-137 gitlab-org/gitlab!73742 (merged) This was a large MR to update the audit events filtered search to use the username instead of the user ID as the key identifier - a small UI change that required a large code change. To make this easier to review I added several notes to explain my changes and the inner workings as this is a pretty complex component. This was the second of 2 MRs and my intention for not splitting it further was because I wanted to create change where the UI impact could be verified. In the end the MR passed reviews went smoothly and there was no changes required before merging.
Add status checks merge request widget extension 283-7 gitlab-org/gitlab!74200 (merged) This was the final part in migrating the merge request status checks widget to the new and work-in-progress widget extension framework. I believe this was the one of the first widgets to be migrated to the new framework. At the time the framework didn't fully support the status checks widget, and all changes were planned by me and made in the framework using this epic.
Restore namespace requirement for project deletion confirmation 33-17 gitlab-org/gitlab!73176 (merged) This change was pushed in response to a high severity issue in order to prevent accidental deletion. It was redoing a change that had been merged, and then undone. The review went smoothly and most of the comments were focussed on the modal's UX.
Update approval settings from boolean to object format 188-126 gitlab-org/gitlab!69505 (merged) This MR updated the approval settings from simple booleans to objects. This was required because with an upcoming change approval settings needed to be enforceable, so a simple boolean toggle would not suffice. It effected the approval settings in user / namespace projects, group projects and groups. The MR was merged with no required changes.
Update approvals store to be reusable 156-132 gitlab-org/gitlab!66839 (merged) This change was another in the series of MRs to update the merge request approval settings. This took the existing group approval rules store and made it reusable so that it could be shared with project approval settings. This saved us from having to write and maintain two separate stores. The change only required one minor typo fixed before merging.
Update createStore to allow for multiple approval modules 53-39 gitlab-org/gitlab!66953 (merged) This was yet another merge request approval settings change. This MR enabled approval store to have multiple modules, one for each level of approval rules (group, project, merge request) and enabled us to reuse a lot of existing code. The change was merged as-is with no required changes.
Update the project approval settings section to Vue 320-128 gitlab-org/gitlab!66536 (merged) This change was also related to the other approval setting MRs. This MR migrated the existing HAML view for project MR approval settings to a Vue app using a lot of shared code from group-level approval rules. The reviews went smoothly and the change was merged with only 2 frontend changes required.
Enforce group approval settings in projects 198-122 gitlab-org/gitlab!69750 (merged) This was another follow-up to the previous MR in this list. It was a fairly large rework of this existing project approval settings since it also included a change to use the new API to get the enforced state on settings. The frontend reviews went smoothly and it was merged with only one change required.

Examples of challenging situations

MR Title Changes Link Comments
Update approval settings to show a toast on success 21-55 gitlab-org/gitlab!77975 (merged) This MR contains a example of how I try to handle disagreement. In this instance I could tell that neither I nor the reviewer had it 100% right. I openly shared my thoughts, added the reviewers suggestions in a separate commit so that the maintainer could compare and left the thread open for maintainer visibility. The feedback from the maintainer proved very helpful and the issue was quickly resolved.
Add merge request violation component for compliance reports 248-44 gitlab-org/gitlab!76214 (merged) This was one of my own MRs that I've already listed in this description, but I also wanted to mention it here since it was a challenge to develop this without a backend API. I had to work closely with the engineer(s) implementing the backend to set the frontends expectations and form a contract.
Use shared runners constants from the backend for the frontend 81-67 gitlab-org/gitlab!72532 (merged) During the review I made a suggestion to avoid provide/inject in that scenario. This led to a back-and-forth between the author and myself. What I should have done was to confirm with the docs before I made the comment to see if it's recommended.

In general I try to avoid situations becoming challenging ones by being proactive. When reviewing or submitting changes I assume best intentions and try to ask questions instead of simply making statements, and also clearly explain my reasoning or point of view with to our handbook / guidelines to avoid confusion. Even if I have a strong opinion I still try to remain open to counter points. Lastly I try to ask for additional opinions or a domain expert if I feel that neither party has a clear answer.

@gitlab-org/maintainers/frontend please chime in below with your thoughts, and approve this MR if you agree.

Once This MR is Merged

  1. Create an access request for maintainer access to gitlab-org/<project>.
  2. Join the [at]frontend-maintainers slack group
  3. Let a maintainer add you to gitlab-org/maintainers/frontend
  4. Announce it everywhere
  5. Keep reviewing, start merging 🤘 😎 🤘
Edited by Jiaan Louw

Merge request reports