Skip to content

Add Robert Hunt as frontend maintainer of GitLab

Robert Hunt requested to merge rob-hunt-frontend-maintainer into master

Self-assessment

After >2 years at Gitlab and over a year as a trainee maintainer I want to propose myself as a ~Frontend maintainer. In this merge request description I have listed for you my justifications and the relevant 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. I also am comfortable with and often submit MR's for 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 requests, projects, group and admin-level tools and reporting. I have also taken on large migrations such as refactoring the approvals form structure and extending to implement status checks. I also added a new DB seed to autopopulate the compliance dashboard to make it even easier for designers and non-compliance engineers to test our work. Lastly my approach to implementation plans for issues has now become the standard for the Manage::Compliance group.
  • 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 follow the conventional comments pattern and contribute to the extension to make it easier to use on GitLab. So I mindful when creating feedback to highlight non-blocking and blocking comments. If I can I would prefer not to create a blocking comment and favour creating follow-ups instead. Especially if a suggested change is outside the scope of the original change. The overall goal here is to keep up the author's momentum and maintain a high-quality high-throughput environment.
  • 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, always doing my best to have short toes. I will always thank the person giving feedback because I appreociate the work they’ve put in considering the code I have produced.

Links to Non-Trival MRs I've Reviewed

MR Title Changes Link Comments
Update oncall grid UX 174-43 gitlab-org/gitlab!55807 (merged) This MR was well outside my usual domain knowledge. Having not been aware of the On-call feature previously, it took me awhile to read up on the history and objectives of the feature before then looking at the new UX improvements.

I was able to test this locally and in doing so provide detailed testing and noted one of the fixes could be simplified as a result. I was also able to identify a set of changes that weren’t in the MR description and a side-effect that was untested.

The maintainer had domain knowledge and noted that the day calculations needed to be tweaked.
Convert Commit dropdown to Vue 395-287 gitlab-org/gitlab!56142 (merged) This MR was non-trivial because of the number of changes made. The changes spanned Vue, JS, SCSS, Ruby, HAML, QA, RSPEC and Jest.

The changes worked really well and were definitely a net-positive with the UI improvements being made. I noted some styleguide issues which were straightforward for the author to resolve. I also found an optimisation to a fragile spec to make it more robust against future changes. It was merged as-is.
Blob refactor: Add ability to lock/unlock a blob 309-15 gitlab-org/gitlab!67408 (merged) Not only was this a fairly big MR but it was also outside my usual domain knowledge. I suggested some follow-up changes for quality improvements. The blocker I noticed in this MR was that we had EE content in CE being optionally rendered by checking for a specific license rather than having the EE content in a separate EE component.

It took two rounds of reviews but in the end resulted in only a minor maintainer feedback to move a translation into the i18n object.
Copy package components to new folder 2799-0 gitlab-org/gitlab!66313 (merged) This MR was impossibly large and therefore pretty much impossible to test manually. The author noted that all this MR did was copy the files from one place to another and they weren’t being used yet.

In this instance I had to rely on the test suite we had and I also ran git diff comparisons between the locations being copied to make sure there were not unusual file changes taking place between the two.

I provided the commands I ran and their outputs to the maintainer for their own use and to compare their findings with my own. It was merged as-is.
Add admin users table row components 375-11 gitlab-org/gitlab!51325 (merged) I had good domain knowledge with this MR and having worked closely with Jiaan many times, I had a good feel on how he worked. So although this was a big MR with lots of actions to test, I knew what to look out for.

Overall, there were some small code quality suggestions and the idea to move one of the utils out to the wider codebase as it could be very useful to others. The maintainer suggested a change to a variable name.

Smaller Reviews with No Maintainer Additions

MR Title Changes Link Comments
Add pronunciation to GitLab profile page 101-1 gitlab-org/gitlab!67111 (merged) This was a small review from a frontend perspective but such a great feature to add!

I noted that we could simplify the HTML and classes a bit to make the overall HAML cleaner and suggested title casing the text.
Fix contribution analytics mr closed count param 1-1 gitlab-org/gitlab!66805 (merged) A super small MR for a bug fix. However I realised that the file being fixed had no tests associated with it at all. As this was an existing issue and the MR was solving a production-level problem, I suggested creating a follow-up to add the tests.

This approach made the most sense to me as it would allow the author to take the time needed to produce detailed, well-rounded tests without the stress of a production-level issue in the back of their mind.
Make repository features promotions translatable 36-9 gitlab-org/gitlab!66811 (merged) The MR was adding internationalisation to the repository feature promotion text. I noted one of the translations was not using the same namespace as the rest and suggested it was updated to match.
Add a locked state to approval settings checkbox 118-17 gitlab-org/gitlab!66450 (merged) I had good domain knowledge of this change so I was able to quickly and easily review this MR. I suggested splitting a component prop up to make it clearer and reduce side-effects and to add a unique ID to each checkbox rather than using ref magic.
Fix labelFilterParam value in issuable_list_root 2-1 gitlab-org/gitlab!65949 (merged) A super small MR for a bug fix. However I realised that the MR didn’t have a test to confirm that the bug had been fixed and to catch the issue if it happens again.

The author added a feature spec to make sure it is covered in future.
Enhance image extension to support image uploads 334-4 gitlab-org/gitlab!65763 (merged) A larger but straightforward MR thanks to the author providing links to documentation and great MR notes!

I noticed that we were adding some regex to test for image input and the regex didn’t support any symbols, only alphanumerical characters. This prompted the author to realise that the image processor already provided an image input regex. So we didn’t need to roll our own and this meant we could remove it!
Update e2e selectors for iteration sidebar & unquarantine test 47-40 gitlab-org/gitlab!63240 (merged) A QA MR to fix a quarantined test. This MR moved around some QA selectors (adding, moving, deleting) to make sure the test would pass consistently. I noticed that in doing so, there was a container that could now be removed.
Fix spacing in blocks on the issuable sidebar 43-46 gitlab-org/gitlab!61650 (merged) This MR modified the sidebar that is seen across large swaves of GitLab. As such I pushed to make sure that Quality got involved to review all the changes and that we ran the full FOSS/RSPEC test suites. As the sidebar is used everywhere, I felt the need to be super-cautious that we didn’t break anything unintentionally that would be seen by the majority of users.
Lock a newly created item card in boards 88-21 gitlab-org/gitlab!61958 (merged) A straightforward MR which was well coded and of high quality. My only non-blocking nitpick was to move some complicated logic into a computed prop rather than having it in the template which the author agreed with.

Links to Non-Trivial MRs I've Written

MR Title Changes Link Comments
Create a new merge request violations compliance report application 621-30 gitlab-org/gitlab!75015 (merged) This MR was the first MR I’ve ever written which used a full client-side GraphQL resolver. This was done to allow us to build out the frontend of the compliance report without a finalised backend (working on that right now!).

It was also the first time that anyone within groupcompliance had worked with client-side resolvers too. So this involved a lot of communication and tweaking to get it to a point that we were all happy.
Add GPG keys list view to Admin Credentials Inventory 259-45 gitlab-org/gitlab!54429 (merged) One of the early instances where I needed to work across disciplines to complete. This involved building the GPG keys authentication and helpers to render them. This also required me to come up with the UI/UX of the view working closely with our UX designer to make sure I followed best practices.

It was also one of the first times I got to work with feature specs!
Updated approval settings to use local form data to stop mutating outside the store 194-40 gitlab-org/gitlab!59887 (merged) This MR came as a result of a previous piece of work I created and we realised that we were mutating outside the store 🙈.

This involved updating our VueX guide and helpers to allow us to retrieve a deep root for mapComputed by allowing us to pass a function instead of a string.

Doing this allowed us to make sure the mutations happened within the store and handle the store update process with ease.
Refactored the rule_form.vue validation code structure 118-91 gitlab-org/gitlab!56961 (merged) An MR part of a multi-step refactoring process. This was particularly difficult as to allow the forms to be more flexible we needed to refactor how the form was structured. I updated the code to follow our latest style guides and changed how the logic was structured to match how we on-average currently implement it.

Breaking this work up allowed us to improve the code quality in this area, making it less fragile and easier to understand. And allowed us in the next step to replace all the hardcoded form groups and inputs with GitLab UI components.
Create a shared form for compliance frameworks creation and updating 431-0 gitlab-org/gitlab!52475 (merged) A part of a 3 step MR. All 3 required thinking about how best to structure the forms and their processing in a logical and DRY way. The best path forward I decided was to create a unified (shared) form to handle the rendering of the errors and fields and then use prop syncing to propagate the data changes up to the parent forms which then handle the specific data processing for creating and editing.

This approach felt clean and also kept the scope of the components to specific tasks and also made it extensible if we needed additional forms.
Create a shared color picker component 142-0 gitlab-org/gitlab!48606 (merged) In close collaboration with UX counterparts, this MR created a color picker component that is Pajama’s compliant and self-contains the color picker logic. This component also has an associated GitLab UI issue but is waiting on the Pajamas finalisation. In the meantime, anyone creating a color picker within the GitLab codebase can make use of this color picker and get all the needed functionality.
Update Compliance Dashboard to use a grid layout 479-255 gitlab-org/gitlab!32440 (merged) One of the first instances of CSS Grid in the GitLab codebase. This MR added CSS Grid to the Compliance Dashboard to help keep the UX spacing we were looking for. It also meant that the responsiveness of the view was greatly improved.

We are currently in the process of replacing this with a whole new table structure to render every individual compliance violation.
Add Snowplow tracking to audit events date range segmented control 47-7 gitlab-org/gitlab!66443 (merged) Not a difficult MR but one I wished to highlight. This MR added tracking to the audit events date range segemented control. Within groupcompliance we were putting a lot of time and effort in building and bug fixing this feature but we weren’t sure if anyone was actually using it. I proposed that we find out.

After a few months of data, it became clear that the usage was super low and as a result, we’ve decided to remove them rather than continue to put engineering and UX effort in. Instead we will focus on improving the other controls and make the UX that much better!

Examples of challenging situations

MR Title Changes Link Comments
New partial for SSH Key Deletion 34-5 gitlab-org/gitlab!50825 (merged) This MR was one of the first instances where I needed to guide a community contributor through the MR process. I needed to guide the community contributor through where the partials were being rendered and how to use GDK or Gitpod to test their changes. I also acquired the other reviews they would need for a successful merge.

In the end, the work produced was of a high quality and the community contributor seemed happy with the support given and getting their work merged. The maintainer approved as-is.
Fix layout issues for project tokens in credentials inventory 13-15 gitlab-org/gitlab!80273 (merged) After testing and approving the initial MR, the author noticed some visual issues once real-world data was added. This is something I should have noticed as a domain expert and I had been working closely with the backend author on the changes.

Once the author noticed the issue I provided an initial implementation plan for them and then did thorough testing of the provided solution, providing an additional patch as a result. As such, I was able to make sure we covered all the table cells and protected against other UI regressions as a result.
Migrate Edit Environments Form to Vue 207-10 gitlab-org/gitlab!66573 (merged) As an example of how I do my best to have short toes: I made a suggestion that the component could make use of GlAlert which is the preferred approach to using createFlash (note: we now have createAlert!). The author prefered not to make this change to keep the MR changes minimal.

Although I would have preferred to make the change while we were working on the file, I could understand the hesitancy and accepted that it was a valid point and I didn’t want my personal preferences to get in the way of an overall net-benefit MR being merged.

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 Robert Hunt

Merge request reports