Remote Development Beta - Review and merge rails remote_dev integration branch (#10258) · Epics · GitLab.org

Remote Development Beta - Review and merge rails remote_dev integration branch

[[_TOC_]] ## Overview This epic represents the effort to merge the Remote Development rails monolith integration branch merge request (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/105783+). Merging an MR of this size was not part of the original plan, but in order to get the initial release of the Remote Development feature out by %"16.0" as leadership has requested, our velocity metrics showed that we had to cut scope significantly. So, the scope we decided to cut was the traditional process of breaking this down into small, iterative MRs, with a thorough but time-consuming, many-cycle, async review process. See more details on the original plan and justification for this new plan in the sections below. ### Background The Remote Development team intentionally made a decision to do all of the initial work on this feature on an integration branch. This was a greenfield feature, with a large scope, many unknowns, many new problems to solve, and many direction changes along the way. Using a team integration branch pattern and low-ceremony process allowed us be very efficient, and go from no code to a releasable feature in less than four months. Here is the documentation of our process for working on the branch: https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/integration-branch-process.md#process-for-new-feature-work-on-the-integration-branches **_It is absolutely certain that we would not have been able to deliver this amount of functionality in this time, and meet the %16.0 initial release goal, if we had followed the standard GitLab high-ceremony, time-consuming process of small iterative reviews against master._** As it is, we had to work very hard to finish just the minimal necessary work related to security, error handling, feature flagging, and basic functionality prior to the beginning of the review process. It had always been our plan to break this down the work on the integration branch into small, cohesive, iterative MRs which would follow a normal review process. We had a plan carefully documented of how this process would work: https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/integration-branch-process.md#master-mr-process-summary However, as the deadline approached, it became clear based on our velocity metrics (more details below) that we would not be able to follow this process and still make the %16.0 release goal. So, the new plan was to merge the integration branch MR as-is. That is the work represented by this epic. Even though this is a very large MR to merge, we believe the quality of the code and tests is high with low risk (more details below). The primary areas of focus in the MR review will be AppSec/security/auth and performance, and ensuring that all functionality can be immediately disabled via feature flags. If we focus on these areas during the review, we should be able to proceed with the initial release with minimal risk, then focus on cleaning up remaining technical debt on master MRs after the release. ### Justifications for merging the integration branch MR without breaking it up 1. It was clear we could not make the %16.0 release without cutting scope: 1. The Remote Development team has been [following a velocity-based XP/Scrum style process](https://about.gitlab.com/handbook/engineering/development/dev/create/ide/#-remote-development-iteration-planning), which allows us to predict how many points we will be able to deliver per iteration, based on our average historical velocity. 2. When we created a report (see code for the report in [this MR](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118436 "Velocity-based agile planning report")), it was clear that the only way we could deliver on time was to cut the scope of breaking down and reviewing individual MRs, especially since there was certainly last-minute scope that was yet to be discovered. Here's the report as of 2023-04-24 showing that the currently prioritized issues would not be completed until the end of the 2024-06-04 iteration, well after the 2023-05-14 cutoff for the %16.0 release. 2. Rollout issue 1. [Rollout issue comment from Product leadership](https://gitlab.com/gitlab-org/gitlab/-/issues/398227#note_1361192858) stating that "In 16.0 we are going to release workspaces as a **beta feature for public projects**." 2. [Consensus from Remote Development EM and PM on the rollout issue](https://gitlab.com/gitlab-org/gitlab/-/issues/398227#note_1372559530) approving of the plan to directly merge the integration branch after review. 3. [Slack announcement from EM](https://gitlab.slack.com/archives/C03KE0L9NC9/p1682461397589279) regarding this plan: 1. Just touching base with y’all here as things have changed for us slightly in 16.0 and I wanted to get ahead of the communications. After conducting our velocity calculations, we have determined that our original plan of dividing our work into smaller merge requests (MRs) will not enable us to meet our deadline. Therefore, we will need to submit a single larger MR, even though this contradicts our iteration value. This decision is necessary to ensure we fulfill our delivery commitments for our users in 16.0. We will address any follow-up issues promptly after the merge.Once the MR is merged, we will have our infrastructure set up using our sandbox cluster, which will allow us to perform a thorough smoke test of the feature and identify any outstanding user-facing issues before it becomes public-facing. The longer we delay, the less chance we have to catch potential issues. The merge itself has 100% test coverage for all its competents and we believe it to be in extremely good shape with [@Chad Woolley](https://gitlab.slack.com/team/UQPBVUD3P) ensuring the pipelines are staying green and in sync with master.We plan to begin the reviews on Monday, May 1, with the goal of merging the MR by Wednesday or Thursday, assuming no major issues arise. Our team will be available to work closely with you to ensure that you can test the MR locally and answer any questions that arise during the review process, so that we can maintain high velocity.If you have any concerns, please do not hesitate to let me know. I am happy to schedule a sync call with you directly, and feel free to loop in your managers for transparency. 3. We have considered the risks, and minimizing risk will be the focus of the pre-merge review process. There were concerns raised about the risks of this on an [internal slack thread](https://gitlab.slack.com/archives/C03KE0L9NC9/p1682505782637209). Here is a summary of that thread: 1. Grzegorz: Do we really plan to merge https://gitlab.com/gitlab-org/gitlab/-/merge_requests/105783 into the master? This may not be a good idea. The MR seems to be large. It is unlikely that we will be able to merge this without risking serious problems. 2. Eric Schurter: We’d definitely be open to other ideas for how we can get it merged before 16.0 but the team is working hard to mitigate the risks of merging this large MR. 3. David O'Regan: Thanks for jumping in! We’re always happy to have the feedback from your side and the other Principal Engineers.Regarding the decision to ship this single MR, we understand that it may seem less than ideal, but we’re working with strict time constraints. Breaking the MR down into smaller ones would have been our initial goal, but unfortunately, we realized that we wouldn’t be able to achieve that within our deadline.However, we want to assure you that [@Chad Woolley](https://gitlab.slack.com/team/UQPBVUD3P) has done an excellent job with this MR, ensuring it’s in sync with the master, maintaining a green pipeline, and providing thorough test coverage. Our plan is to take it through rigorous reviews in the Backend, Frontend, QA, AppSec, and DB teams. We’re also open to the idea of having multiple maintainers to share the workload if needed.We understand that this situation is not in line with our values of iteration, but we’re doing our best to balance that with the urgency of shipping the initial release on time.We welcome any suggestions or ideas you may have, and we’re open to discussing them as a team! 4. Chad Woolley: 1. Trust me, it was never our plan to merge this MR directly to master. As Eric and David said, we had a [detailed, disciplined plan of how we would get everything merged into master as individual MRs](https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/integration-branch-process.md#process-for-ensuring-all-integration-branch-work-is-eventually-merged-to-master) However, [based on our velocity metrics, it was clear that this would be impossible to do](https://gitlab.slack.com/archives/C03KE0L9NC9/p1682341781401509) and still meet leadership’s goal of delivering this as a public beta release in 16.0.And leadership from Sid down has made it _crystal clear_ that they want this shipped in 16.0.So here we are, and we are doing our best to meet that goal, with a new plan.Which yes, involves merging this MR directly to master. 2. And while I may not share David’s glowing enthusiasm about the quality of my own code (I know there’s many GitLab coding standards I may not have been aware of, because I have mostly worked outside the monolith or in odd isolated corners during my GitLab tenure), **I do feel this is relatively _low risk_ to merge**: 1. It’s a greenfield feature. 2. It has very little interaction with any other existing code or areas of the app. The only entry points are two GraphQL endpoints used by Vue for create/update, one new internal REST endpoint used by GA4K, and a small addition to the GA4K internal REST config endpoint to add our config logic. 3. It only introduces two new tables. 4. We will have feature flags in place to completely shut it down at all the above API entry points if there are any problems. 5. While I may not have confidence that I followed all of our internal review standards, I do feel very good about the fact that it has a clean, well-organized, layered architecture, with full test coverage of all happy-path scenarios and a green CI which passes all linting. <div> If anything, IMO the most risk is involved in the changes to the GA4K logic and traffic profile, but most of that code has already been merged to master after careful review by Mikhail. And that too can have access completely shut down by disabling the feature flag on the Rails endpoints.So of course it won’t be of perfect quality, and we will have to overlook lots of things we would ordinarily take time to fix in a normal MR review cycle.However, as David DeSanto said in his presentation about our AI efforts, GitLab is facing many challenges, and we need to return to our startup roots and not be afraid to “move fast and break things”.While we have no intention or expectation of breaking things, we are definitely moving very fast to meet the 16.0 release date.After that, we definitely have a plan to regroup, review, and carefully organize and break down our backlog with a path to GA.This will definitely involve addressing any concerns raised during the review process, as well as important technical debt that we intentionally decided to allow to be merged as part of this MR. </div> ---------------------------------------------------------------------------------------- ## Process Although our goal is to have no remaining blocking security or performance concerns when we merge, there will be many areas we need to follow up on after the release. These follow-up tasks have been split into three categories: TODOs, disable linting rules, and any follow-up but non-blocking concerns raised during the review process. These three categories will be organized into the following sub-epics of this epic: 1. Sub-epic for TODO follow-up issues: https://gitlab.com/groups/gitlab-org/-/epics/10472+ (The work to populate this epic is still pending with this issue: https://gitlab.com/gitlab-org/gitlab/-/issues/408398+) 2. Sub-epic for disabled linting rules follow-up issues: https://gitlab.com/groups/gitlab-org/-/epics/10473+ 3. Sub-epic for follow-up issues from review: https://gitlab.com/groups/gitlab-org/-/epics/10474+ ## Reviewer resources 1. The [dedicated `#f_remote_development_mvc` slack channel](https://gitlab.slack.com/archives/C052T8Z39GA) will serve as the main place for coordinating on reviews, and resolving any blockers on the integration branch merge process. 2. General or non-urgent/non-blocking questions may also be asked in the [main `#f_remote_development` slack channel](https://gitlab.slack.com/archives/C03KE0L9NC9). 3. Any blockers or urgent questions can be directly addressed to Vishal Tak (`@Vishal Tak`) and/or Chad Woolley (`@Chad Woolley`) - just `@` mention them in the slack message. Please [avoid direct messages](https://about.gitlab.com/handbook/communication/#avoid-direct-messages) per our handbook guidelines, so that anyone on the team can respond based on availability. 4. There is an open [Zoom room](https://www.google.com/url?q=https://gitlab.zoom.us/j/95426368947&sa=D&source=calendar&ust=1683351224426470&usg=AOvVaw3JWflkKTe0lPJ9E_2uOjKn) and [calendar event](https://calendar.google.com/calendar/event?action=TEMPLATE&tmeid=Mm91MGZna2hxZDI5dTN2NnRhMzBrMGE3ZWsgZ2l0bGFiLmNvbV92ZGc3bW04NDRuczVrN3JxZGlyMzM0N2YwOEBn&tmsrc=gitlab.com_vdg7mm844ns5k7rqdir3347f08%40group.calendar.google.com) which is a long-lived, 24/7 room for ongoing sync collaboration during this week. Collaborators should drop into this room as needed and ping one another in the `#f_remote_development` or `#f_remote_development_mvc` to set up collaborative sessions given the TZ disparity. ## AppSec review 1. The main AppSec review issue: https://gitlab.com/gitlab-com/gl-security/appsec/appsec-reviews/-/issues/201+ 1. The comment on the AppSec review issue which indicates that _"...we are not blocking that MRs from being merged, unless we are aware of an issue that's considered severe enough to not go into production like a `S1` or `S2`"_: https://gitlab.com/gitlab-com/gl-security/appsec/appsec-reviews/-/issues/201#note_1370208351 1. We have a dedicated issue to manage the parts of the AppSec review process from an Engineering team perspective: https://gitlab.com/gitlab-org/gitlab/-/issues/408279 1. All remaining security/auth related issues for the Beta release are captured in the `Remote Development Beta -Auth` epic: https://gitlab.com/groups/gitlab-org/-/epics/10377+ 1. **_At this point (2023-04-30), there are no known issues or blockers to the merge_**. See details in this comment: https://gitlab.com/gitlab-com/gl-security/appsec/appsec-reviews/-/issues/201#note_1373008923 1. There was [one initial question raised so far](https://gitlab.com/gitlab-com/gl-security/appsec/appsec-reviews/-/issues/201#note_1372739808) as part of the AppSec review. Here is the issue to track it ensure we respond to it satisfactorily: https://gitlab.com/gitlab-org/gitlab/-/issues/409317+ ## Database review The Remote Development team has not yet had time to do a thorough self review based on the following resources. We are looking for guidance from the database reviewer, and ready to immediately address any blocking issues. - https://docs.gitlab.com/ee/development/database_review.html - https://docs.gitlab.com/ee/development/database/database_reviewer_guidelines.html ## Ruby code review It is our plan to do a self-review of every line of code at the beginning of the review process, and we are ready to immediately address any blocking issues found by reviewers. - https://docs.gitlab.com/ee/development/code_review.html ## Frontend code review We will rely on our resident `Create: IDE` frontend maintainers Paul Slaughter and Enrique Alcátara to manage the frontend review process. Also, much of the the new Frontend UI code has already been reviewed and merged to master as part of separate MRs. ---------------------------------------------------------------------------------------- ## Previous Description This epic has been repurposed from the original epic which represented the work to split the integration branch into small MRs to merge to master. The original description for this epic is preserved below: <details> <summary>Original description</summary> **Overview** This epic is intended to capture all the work involved in managing the complete lifecycle of the [Remote Development integration branches](https://gitlab.com/gitlab-org/gitlab/-/issues/383997 "Tracking Issue for Remote Development Integration Branches") and ensuring all their code makes it to master correctly and efficiently. **Ensure all work on all remote_dev integration branches is completely merged to master** The work includes (but is not limited to): * Everything described in the process here: https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/integration-branch-process.md#process-for-ensuring-all-integration-branch-work-is-eventually-merged-to-master * All review processes and feedback cycles for all master MRs * All remaining work to finish up all remaining work on the integration branches, e.g. ensuring that any remaining TODOs have follow-up issues opened. * Anything other blockers we run into as part of the merge process **More context and docs** - https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/integration-branch-process.md - https://gitlab.com/gitlab-org/gitlab/-/issues/383997+ - Tentative plan for organizing MVC epics and issues: https://docs.google.com/document/d/1TuB40gFxeJuN6UxCsxCKEgEOl6zC8KYhgU4hEd7IGqc/edit **Split of MRs** See ordering of issues on epic, master MR issues are named `Master MR curate/review/merge: ...` </details>

epic