Skip to content

Make Scott Hampton a maintainer

Scott Hampton requested to merge shampton-yml-update-maintainer into master

Summary

I've had the pleasure of working here at GitLab for nearly 2 years now, and after having been a trainee maintainer for half that time I'm opening this maintainer MR. I feel that I know the codebase well and the MRs I author and review consistently make it through maintainer review without major changes. I've listed below many MRs I feel demonstrate this. Please approve if you agree. 🙇

Links to Non-Trival MRs I've Reviewed

Large-ish MRs

MR Title Changes Link Comments
Step component for paid signup flow 523-22 gitlab-org/gitlab!21568 (merged) Asked the author to use Vuex instead of their custom store implementation. I provided guidance for how the folder structure should be laid out. Asked that they use existing utility classes instead of custom CSS. I asked them to stop using toBeTruthy() in a spec. I also suggested a different default prop value.
Add ability to see project deployments at group-level (FE) 512-90 gitlab-org/gitlab!15037 (merged) I suggested a change to clean up a state mapping function. I also suggested that we move some EE only HAML code to the ee folder and use the render_if_exists method, which was backed up by the maintainer. I noted that the author could remove a mounted function since the code inside was not dynamic. I also suggested we remove additional CSS in favor of using existing utility classes.
Port ci lint from HAML to Vue 547-19 gitlab-org/gitlab!43031 (merged) Suggested we handle the possible GraphQL mutation error. Asked that we move some methods to be computed properties by moving the looped items to be their own component. Noted that the GraphQL mutation wasn't returning any properties, but the client resolver was - suggested we add those properties to the mutation file. Also asked to change a spec to avoid checking prop values, and instead testing for rendered elements.

Other non-trivial MRs

MR Title Changes Link Comments
Migrate DiffFileHeader tests to Jest 489-756 gitlab-org/gitlab-foss!32798 (merged) I suggested we change a number of tests that were using toBeTruthy and toBeFalsy to instead use toBe(true) and toBe(false)
Vue-i18n: autofix for app/assets/javascripts/vue_shared directory 138-56 gitlab-org/gitlab-foss!30287 (merged) I noted an error in the use of a sprintf where there should have been a third parameter of false so that the HTML would not be escaped. I provided a screenshot of it being broken due to the current state, and created a suggestion comment for the fix.
Extract BoardScope component 227-121 gitlab-org/gitlab!14556 (merged) I noted that there were some strings that should be using i18n, which was done in a followup issue.
Apply filters to vultnerability count list 46-7 gitlab-org/gitlab!41566 (merged) I pointed out that a test wasn't actually testing anything due to default props.
Add the artifact expiration help url (community contribution) 51-3 gitlab-org/gitlab!39546 (merged) Asked that we use GlIcon instead of font awesome. Helped debug a failing test caused by bad default props. Also asked them to use data-testid for testing purposes instead of selecting by existing classes. Maintainer pointed out we could remove a extra span.
Add cloud provider selector buttons 158-34 https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32302 I identified a couple of places where we were not following our i18n standards in regards to not splitting sentences in links. The maintainer requested an SVG be moved to the gitlab-svg repo and added a missing alt-text to an image.
Show build status in branch list (community contribution) 140-3 gitlab-org/gitlab!30948 (merged) I helped them align an icon by pointing them to a HAML partial I had created, and showed which classes would help center the icon best. I also suggested removing new CSS in favor of utility classes. The maintainer further discussed a detail that I had marked as non-blocking, but ultimately it was left as the author had originally written it.
FE Support batch diff loading 361-27 gitlab-org/gitlab!18544 (merged) I caught a copy error and pointed out that we were using fdescribe in a couple tests instead of just describe. The maintainer suggested that we move some strings to be constants
Convert project graph page to use utility classes for styling 12-86 gitlab-org/gitlab!44145 (merged) I asked them to remove the classes from the HAML code that they had just removed from the CSS so that there was no unused code. A follow-up issue was created due to those classes being used in spec files.

Links to Non-Trivial MRs I've Written

Pipeline Test Report Usability Improvements

The pipeline test report was not performant, and there were some UX concerns as well. The following MRs were part of this larger effort to improve this feature. Note that all of the following MRs were merged either with minor comments from reviewers or with no concerns at all.

MR Title Changes Link Comments
Refactor pipeline test report store 126-71 gitlab-org/gitlab!35789 (merged) Preparing the existing store to take on performance enhancing features.
Change selectedSuite to use index 61-44 gitlab-org/gitlab!36136 (merged) Since we were moving the suites to load on click, the legacy way of handling selecting a suite wasn't going to work. I did an iterative step to prepare for the lazy-load technique.
Use the test report summary 225-73 gitlab-org/gitlab!36349 (merged) Behind a feature flag, moved the test report to load only the summary and not all of the test suites on page load.
Get test suite details on click 124-27 gitlab-org/gitlab!37232 (merged) Changed the test report to load the test suite cases on click instead of on page load, making the whole section load faster.
Remove build_report_summary feature flag 56-393 gitlab-org/gitlab!37629 (merged) Removing the feature flag that was hiding the performance improvements. To quote the maintainer, "Great job, pretty much every file this MR touches looks cleaner now".
Format test report times to adjust for ms 52-8 gitlab-org/gitlab!39644 (merged) The duration times for jobs were not displaying milliseconds. We adjusted the display of that column to account for milliseconds.
Remove junit report feature flag 92-178 gitlab-org/gitlab!39260 (merged) The moment we were all waiting for - finally removing the feature flag that was hiding the entire test report.

Other notable MRs

MR Title Changes Link Comments
Create store for a11y MR widget 500-10 gitlab-org/gitlab!29199 (merged) An iterative step towards creating the accessibility MR widget. Created the Vuex store for the widget. Reviewer pointed out we could pass the REST endpoint when creating the store. Maintainer pointed out a place where I could refactor code to be more DRY and create a constant string.
Fetch more group projects 179-19 gitlab-org/gitlab!43044 (merged) GraphQL fetchMore query for more group projects when dropdown menu is scrolled to the bottom. Reviewer noted some spots where we were doing extra if checks. Maintainer kindly taught me about error handling with Apollo, which was very helpful.
Select projects for group code coverage report 395-55 gitlab-org/gitlab!42129 (merged) GraphQL - creating a modal to allow the user to select the projects and date range for a test coverage report. Reviewer had me move a repeated function to be util function and noted a minor adjustment for a test. Maintainer helped with provide/inject properties and noted that I should move the changelog to the ee folder.
Update grouped test report spec 152-195 gitlab-org/gitlab!34089 (merged) Updated the spec files for the grouped test reports app after they were migrated from karma to jest. Was merged as-is.

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