Make Florie Guibert a Frontend Maintainer
Self-assessment
It's hard to specify hard requirements for becoming a maintainer, which is why the documentation consists of flexible guidelines. Nevertheless, it's still important to have a more formal list to help both a candidate and their manager to assess the "readiness" of the candidate.
-
Advanced understanding of the GitLab project as per the documentation: good feel for the project codebase, expertise in one or more domains, and deep understanding of our coding standards
-
The MRs reviewed by the candidate consistently make it through maintainer review without significant additionally required changes. -
The MRs authored by the candidate consistently make it through reviewer and maintainer review without significant required changes.
I haven't been a trainee for very long but I've been reviewing for much longer. I've also been a gitlab-ui
maintainer for a few months which was helpful as a stepping stone toward becoming a maintainer for the main codebase.
Special thanks to @ekigbo for mentoring me these last few weeks and helping with this process.
Examples of challenging situations
I cannot think of any particular awful situation I've been in. When I introduce a bug, I'm never on my own and I always feel supported. If a revert is needed, I have no issue doing it. Sometimes the maintainer who merged the MR introducing the bug revert it themselves. Then I go back to it, create a new MR and ensure the bug is gone in the new version.
Sometimes reverting is not what makes the most sense and I'll decide to quickly submit a new MR fixing the issue, working closely with the maintainer who reviewed and merged the original MR.
So what I've learned, is that we're never single highhandedly responsible for mistakes happening. Maintainers are humans too and we all make mistakes. We're together in making them, and we're also together in fixing them, which is something I truly appreciate here at GitLab. It's probably why I've never panicked when something bad happens. I jump on the incident channel or on a call with a maintainer and we fix the issue.
This MR is an example of a change that introduced a bug. It made it impossible to create a board with an iteration. With it being behind a feature flag (and without letting anyone have time to realise there was a bug), I submitted a fix and sent it straight to the maintainer of the initial MR.
Links to Non-Trival MRs I've Reviewed
MR | Diff | Key takeaways |
---|---|---|
PipelinesUsageApp: add project list and their CI usage | 627 -36 | This had been through an initial round of review so I thought it was interesting that I still had feedback to add to it. It's quite a large change (>600 lines) and touches on different aspects of the code (vue, ruby helpers, graphql). Some tests were missing and I made some suggestions to improve readability. I was mostly confused about the use of Strings for Booleans and had to double check that those variables could never be anything but true/false before suggesting to cast them. This is quite outside my domain knowledge as well. |
Update issuable confidentiality UI & status text | 222 -41 | This MR contained a decent amount of code repetition and before suggesting to create shared components, constants and utility functions, I had to make sure it made sense. |
Deprecate manual iteration management for automatic iteration cadences | 334 -237 | This is quite a large change (>300) that involves a feature flag. A lot needed to be tested to ensure everything was working as expected with the feature flag on and off. It also involved the use of Vue Router which I'm not familiar with. |
Update project compliance framework settings to be more clear | 297 -38 | This is very much out of my domain knowledge and a decent size change (~300). It involves the addition of a Vue component while making a lot of changes to a related HAML template. The mix makes it harder to review than having all the changes in the same language in my opinion. |
Remove jest test callbacks from API specs | 215 -251 | |
Remove jest test callbacks from IDE specs | 763 -1176 | These two MRs were hypnotising to review as they are large repetitive changes. It becomes hard to pin point mistakes after 20 minutes of reading what seemingly looks the same. |
Generalize epic filtered search component for board reuse | 314 -221 | There was a decent amount of code to tidy up there, while also ensuring existing uses of the component were still working as expected. |
Trainee issue for more: #12875 (closed)
To summarise, here are the things I look for and feedback I tend to leave:
- I ensure the description is clear enough, with screenshots/videos, steps to reproduce and potential mention of a feature flag. It's happened many times that a feature flag was involved without its name clearly written in the description, making things harder to test.
- I ensure that required counterparts are involved when needed (notably QA and UX)
- I manually test the change locally. The most important things to me is that it works, the code looks reasonable and there's no performance impediments.
- I tend to leave comments about migrating bootstrap CSS utility classes to gitlab-ui classes and avoid adding too much new CSS.
- I ensure tests coverage is good and that they make sense. I always have this epic handy when making suggestions on how to improve tests so we don't keep using old standards.
Links to Non-Trivial MRs I've Written
Note: Building the frontend for Swimlanes while being the feature's DRI is probably the least trivial thing I've done but there are too many MRs to list and it's within Plan domain knowledge so here is the related epic. The MVC work itself spanned over 9 milestones.
Note 2: Most of the features I work on as part of the Plan stage are quite complex, so this is just a selection to demonstrate various aspects I've been working on.
Label component migration
MR | Diff | Comments |
---|---|---|
Replace labels in Vue with gitlab-ui component | 158 -368 | This was a huge undertaking, which I started very early after joining GitLab. Labels are everywhere and it was the very first gitlab-ui component to be integrated into the main codebase. I learned a lot in a short amount of time about our codebase and the technologies we use. |
Update labels on sidebar update with gitlab-ui css classes | 55 -26 | |
Replace labels in Haml with gitlab-ui CSS | 285 -158 | |
Docs on integrating components in GitLab | 19 -0 | Since this was the first gitlab-ui component to be migrated in GitLab, I wrote some documentation about the process to help engineers with migrating other components. |
Sidebar widgets
MR | Diff | Comments |
---|---|---|
Sidebar notifications subscriptions widget | 536 -198 | All these MRs are similar in terms of architecture while varying in terms of effort. I participated in the effort to refactor and migrate sidebar components to be standalone reusable components. By owning the queries and mutations, they increase performances of pages with sidebars. They are also versatile components that can be used for Issues, Epics and Merge Requests. This was an amazing learning experience, with GraphQL and Apollo mostly. |
Due date issue sidebar widget | 392 -42 | ------ |
Add To Do sidebar widget | 466 -31 | ------ |
Milestone sidebar widget | 1075 -344 | ------ |
Milestone widget for Issue and MR page sidebars | 165 -72 | ------ |
Labels widget
MR | Diff | Comments |
---|---|---|
Tidy up labels widget | 35 -1057 | Labels was the most complex widget and I wasn't the only engineer involved. I took over the development of this component after Natalia did the heavy lifting. This MR cleans up legacy code from previous labels selector. |
Labels widget architecture polish | 48 -70 | Some refactoring so that the widget follows the other widgets' architecture |
Clean up labels_widget feature flag | 140 -658 | Removes the feature flag once the widget was usable. |
Add Labels widget support for epic | 227 -87 | Extends the widget to make it possible to use it to assign labels on epics. |
Use labels widget on epic board and MR sidebar | 222 -57 | Migrates selector to widget in more places. |
Labels widget - Fix search input | 311 -133 | UX polish to improve usability |
Board sidebar - Migrate labels select to widget | 177 -62 | Migrates selector to widget in more places. |
Consolidate labels widget architecture | 282 -119 | This was the most complex widget to build and migrate and it was definitely a team effort. It required a lot of iterations and polish but having it behind a feature flag for a while really helped with this process. It was easier to find and fix bugs once it was in front of users though. A few steps like this one were just some non user facing refactoring to align its architecture with our code guidelines and other widgets. When we started this work, we just copied the existing selector as it was already a complex composition of Vue components. But it was old and needed some serious refresh which could only be done over multiple iterations |
Documentation
MR | Diff | Comments |
---|---|---|
Work items widget frontend architecture | 114 -0 | I used what I learned when building and migrating sidebar widgets to make some architecture recommendations about how we are going to build widgets for work items |
Epic boards documentation | 68 -0 | Just feature docs |
Epic boards docs update for 14.0 release | 195 -58 | Just feature docs |
Update docs for Milestones in Roadmap | 27 -20 | Just feature docs |
Roadmap settings documentation | 18 -0 | Just feature docs |
Introduce Cypress e2e test in gitlab-ui
MR | Diff | Comments |
---|---|---|
RFC | I've taken an interest in gitlab-ui pretty early on in my time at GitLab. As a contributor it became obvious that it was necessary to add the ability to test complex components before they get integrated into our main codebase. It was not hard to convince the frontend team that testing compositions of components would make our library more robust and would prevent some issues when integrating them into GitLab. | |
MR | 547 -26 | This MR introduces Cypress, documents the process of adding tests and adds a pipeline job to run it. It adds tests for one component as an example. The Frontend team happily adopted Cypress and our end to end test coverage in gitlab-ui is progressively increasing. |
Some roadmap contributions
MR | Diff | Comments |
---|---|---|
Add hierarchy depth to roadmaps | 1045 -262 | This was my first contribution to Roadmap, which is quite a complex feature. Learned a lot about it, as well as epics. |
Add milestones to roadmap | 1467 -82 | This was before I knew about feature flags. I was still of the school of avoiding submitting MRs without adding user value. After this gigantic MR, I learned to iterate while being behind a feature flag. |
Update docs for Milestones in Roadmap | 27 -20 | Feature docs update for the above |
Other
MR | Diff | Comments |
---|---|---|
Rspec wait_for_requests helper waits for graphQL requests | 42 -11 | I made this change following recommendations of a backend engineer but it was super interesting to make changes to rspec helpers. This was critical for me to be able to write end to end tests for the features I work on which often use GraphQL. |
Once This MR is Merged
-
Create an access request for maintainer access to gitlab-org/<project>
. -
Join the [at]frontend-maintainers
slack group -
Let a maintainer add you to gitlab-org/maintainers/frontend
-
Announce it everywhere -
Keep reviewing, start merging 🤘 😎 🤘