Skip to content

Make Frédéric Caplette a FE maintainer

Frédéric Caplette requested to merge make-frederic-caplette-a-maintainer into master

Hello fellow frontenders!

This is my MR to transition from a trainee maintainer, to a full-fledged, top of the line maintainer. I've been a trainee for about 6 months now and I've s̶e̶e̶n̶ ̶t̶h̶i̶n̶g̶s̶ reviewed quite a lot of diverse MR by now. As months went by in my training, I have improved on multiple facets of my reviews and I am at the point where maintainers rarely add significant changes to my points.

During my time as a Trainee, I've learned a lot of our codebase, guidelines and diverging opinions. I try to respect the different approaches and let suggestions be suggestions whenever I feel they are non-blocking. I want to respect everyone time while enforcing our standards and quality where it matters (which I got better at over time 😅)

For those who have worked with me as a trainee, I hope that working with me was sufficient to convince you that I am ready for this transition, but nevertheless, the list of my MRs has been added here so feel free to dig in and let me know what you think and how I can improve!

Notes:

  • You might notice in the MRs I've listed that there isn't much VueX in there. Well truth is I've reviewed a lot more GraphQl than VueX for state management. When I did review Vuex, it was always boiler plate and very rarely had anything to add.
  • This MR is only to add me to gitlab project and not gitlab-ui. Why? Because even though I did review a couple of gitlab-ui MRs, I do not feel like I have worked enough in it to be a maintainer, which I will work towards after this one 😬

Links to Non-Trival MRs I've Reviewed

MR Changes Link Comments
Pipeline Graph Structural Update: Add New Links Component 495-78 gitlab-org/gitlab!51594 (merged) A large MR that was touching the pipeline graph. In the highlights here, I pushed to add some safeguard on some properties getter. I also made sure the naming was on point to communicate what was going on.
Show API errors when a command-only comment fails 146-8 gitlab-org/gitlab!55457 (merged) A small MR with a lot going on. I had some suggestions to refactor part of the MR for maintainability. The assignee was happy to implement all my suggestions 🎊
Support extensions as configurable ES6 classes 247-65 gitlab-org/gitlab!49813 (merged) Fun, pure JS review here where most of the work was making sure I understood the impact of the change and how we would use this in the future. This one is a good example of an Mr where the work was not pointing out problems, but understanding the impact of the change and making sure I understood it all properly. Got a shoutout from the maintainer and the reviewee in our frontend call! 🕺
Introduce jobs_table_vue feature flag with blank table 567-19 gitlab-org/gitlab!57155 (merged) A couple of points I raised were for maintainability and also some cleanup regarding graphQL queries. This was merged as-is.
Add query string utilities to package and registries 260-13 gitlab-org/gitlab!57084 (merged) I am adding this MR to the list because it shows the kind of detail I put in my question/suggestions.
Keep latest artifact setting - UI 285-1 gitlab-org/gitlab!49500 (merged) Graphql update where I caught a double query because of an unnecessary refetch, which started out as a question to the author to let them first the chance to tell me if there was a specific reason for this. I also pointed out using rails utils to build the doc path.
Show code quality badge on files in MR diff view 346-17 gitlab-org/gitlab!56710 (merged) There were a few points, some around visibility polling, follow-up for the eventhub and avoiding tests on the vm. I worked with the author by giving clear suggestions around larger comments I had and I was flexible around sections that would most likely require a lot of additional work to be defer in a follow-up and not block a great improvement.

Clean Cats (Few to No Maintainer Additions)

MR Changes Link Comments
Kebab fork slug when name is changed 39-1 gitlab-org/gitlab!55599 (comment 521344383) An instance where I worked with the assignee to remove assertion in our tests on the vm and took the time to propose alternatives and even sent a small patch to propose an implementation to help demonstrate how my suggestions could work.
Connect Registries searches to URL 269-52 gitlab-org/gitlab!57251 (merged) Mostly some readability comments and one main comment around refactoring a method in mounted lifecycle which resulted in a nice refactor from the assignee 🎯
Refactor pipeline header to use GraphQL mutations 85-51 gitlab-org/gitlab!47011 (merged) It was a small graphQL MR, but I pointed out a way to make the code leaner and got no additional maintainer comments. Woah
Highlight paid features during active trial - Part 1 222-3 gitlab-org/gitlab!56155 (merged) There were a couple of questions and good conversation in here that I feel were beneficial to the assignee and to myself 😄 A lot of my points were from our guidelines, not so much on the code structure itself.
Fix polling bug on pipeline header 33-1 gitlab-org/gitlab!47301 (merged) Another small graphQL one, but I made 1 major technical suggestion that made the polling much simpler to handle and another one to clean up the watcher. Maintainer had some additional questions, but nothing was really changed after 🎊
Validate group names before importing 222-71 gitlab-org/gitlab!54927 (merged) This one was merged as is. I had a couple of comments, mostly semantic improvements that I felt would help understand this bettr in the future and also use computed props we already had.
Fix unified components inline view 37-5 gitlab-org/gitlab!47345 (merged) In a very small diff, I found a small unnecessary default value that ended up cleaning up the code a bit. I could have pointed out that we needed additional test coverage though 🙈

Community Contributions

Here are some examples of my CC reviews. They were always simple, but it shows how I interact with our community.

MR Changes Link Comments
Resolve "Replace bootstrap alerts in ee/app/views/trials/_banner.html.haml" 11-6 gitlab-org/gitlab!41074 (merged) I mostly made sure the changes fit with our design and I advocated for not making the contributor redo their whole work twice after they received contradictory information from Gitlab team members.
Apply new GitLab UI for buttons in pipeline page 11-6 gitlab-org/gitlab!53364 (merged) Change to gitlab utility classes. I let the community contributor know that they should add screenshots in context of their change while also informing them of what the next step could be for their work without blocking them

Links to Non-Trivial MRs I've Written

Pipeline visualization

This spawned from a side project Sarah and I wanted to do and after demoing it to our CI/CD call a quick preview of what this could look like. After that, we decided as a group we wanted it in the platform. I initially implemented it in 3 MR as seen below. Once this was done, we decided to create our own section, now known as the Pipeline Editor https://gitlab.com/gitlab-com/www-gitlab-com/-/ci/editor and so the whole functionality was moved there. We also change the code and defered to the backend to give us a parsed CI/CD config instead of parsing it on the frontend. Here is the journey of all of that in code.

Here a nice example of the visualization for www.gitlab.com Screen_Shot_2021-04-19_at_12.22.02_PM

MR Changes Link Comments
Visualization of .gitlab-ci.yml static file (MVC) 637-1 gitlab-org/gitlab!40880 (merged) The first part was adding a YAML parser to get the data from the CI/CD config file and then create a column-based graph similar to what we already have in the pipeline/id view, but that would serve as the basis to represent a the yaml static version of your config instead of the one generated at run time.
Implement the drawing algorithm for yaml viz 224-16 gitlab-org/gitlab!44471 (merged) This is the core functionality for the links representation for needs keyword in the graph. Lots of fun SVG drawings 😄
Add hover highlights to CI config graph for needs jobs 355-159 gitlab-org/gitlab!45066 (merged) Add the hover to the SVG lines that were drawn based on which job needs which. A lot of performance consideration given the scale of the data we can handle (Users can use over 8k needs, why though, no one knows 😅) There is still work to be done to improve this.
Fix CI config viz for cross-stage lines 9-1 gitlab-org/gitlab!45066 (merged) Small one that I took the liberty of implementing between big work to fix the algorithm that draw lines, some maths involved and I am weirdly proud of it 😆
Remove visualization tab on CI configs 1-342 gitlab-org/gitlab!48635 (merged) Prep work for the migration that removes all the code that lived in the file view. It also removed the yaml parsing on the frontend.
Add graphql query for linting in editor app 87-29 gitlab-org/gitlab!49379 (merged) Preparing the ground to move from the frontend to the backend to parse the CI/CD config.
Add visualization functionality to Authoring home 418-177 gitlab-org/gitlab!48793 (merged) The actual move to the new section and connecting the visualization to use graphQL as data instead of the parsed yaml.

Refactor of Pipeline Editor section

After the initial push to release an MVC of the pipeline editor section, I initiated an architecture plan and refactor to prepare the section to grow organically from its start point. I created an issue to discuss the plan I had come up with with my fellow frontenders in Pipeline Authoring and then I implemented it. I ended up opting for a feature branch because there was too many changes to do organically and I couldn't ship part of it because everything was related.

MR Changes Link Comments
[Part 1] Break down pipeline editor into smaller components 355-206 gitlab-org/gitlab!52596 (merged) The first part was really about the components and code. The idea was to break one big root vue component into several smaller ones that would communicate mostly through events and apollo local queries for shared state. Figuring out how each component would talk to each other and why use one way over the other was the real work (good thing I started in that issue 😏 )
Breakdown tests in pipeline editor app new architecture 598-372 gitlab-org/gitlab!52790 (merged) This was moving the tests so the fun part of this 😅 A couple of small catches from the maintainer, but nothing big.
Merge feature branch for Pipeline Editor app refactor 957-586 gitlab-org/gitlab!52200 (merged) the final nail, merging the feature branch. It also gave a chance to reviewers and maintainers to have one good look at everything together before the merge.

Other non-trivial MRs I am proud of

MR Changes Link Comments
Adds a global status to the pipeline editor section 252-194 gitlab-org/gitlab!55787 (merged) Adds an apollo value for global status in our section (e.g. EMPTY, INVALID, VALID, etc) which we can now use anywhere in the app. There was a lot of moving pieces, but it was done and merged with very few comments from maintainer.
Centralize shared state in Authoring section 209-182 gitlab-org/gitlab!58790 (merged) Use the global status above and add alerts that are reusable for each tab. This allowed us to remove duplicated logic and is also adding some nice UX on top of it 🕺 Also very few maintainer comments.
Show merged version of ci config 471-61 gitlab-org/gitlab!53299 (merged) A new feature to show the merged version of a CI/CD configuration. Feature flags, documentation, Vue, graphQL, specs, few maintainer comments AND helping out a community member It's all here

@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.
  2. Create an access request to be added to 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 Sam Beckham

Merge request reports