Skip to content
Snippets Groups Projects

Add feature flag for CI variables drawer

Merged Mireya Andres requested to merge ma/ci-variable-drawer-ff into master

What does this MR do and why?

For #410414 (closed)

The drawer will replace the modal form for creating, editing, and deleting CI Variables.

This MR is purely for the rendering the drawer under the feature flag. Interactions with the UI (such as validations and submitting the form) will be implemented in future iterations.

The MR is broken down into different commits for: 1) showing the drawer under a feature flag; 2) scaffolding the component with UI elements. The feature flag will be rolled out on a user actor basis because the CI variables setting exists on the instance-level, group-level, and project-level. The drawer/modal component is shared in all three contexts.

No changelog entry yet since this will be developed under the ci_variable_drawer feature flag.

Implementation

Iteration MR
Setup drawer with feature flag :point_left: You are here!
Add form validations :hourglass_flowing_sand:
Ensure that all mutations are working and update QA specs :hourglass_flowing_sand:
Rollout #418005 (closed)

Screenshots or screen recordings

Before After
modal Screenshot_2023-07-18_at_18.37.57

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Enable the ci_variable_drawer feature flag.
  2. Go to Settings > CI/CD > Variables.
  3. Click on the Add variable button or edit a variable.
  4. The drawer should show up instead of the modal.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Mireya Andres

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Contributor
    4 Warnings
    :warning: This merge request is quite big (527 lines changed), please consider splitting it into multiple merge requests.
    :warning:

    featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

    For more information, see:

    :warning: This merge request contains lines with QA selectors. Please ensure e2e:package-and-test job is run.
    :warning: This merge request adds a new rule to app/assets/stylesheets/framework/common.scss or app/assets/stylesheets/utilities.scss.
    1 Message
    :book: CHANGELOG missing:

    If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog.

    If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message.

    If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

    QA Selectors

    The following changed lines in this MR contain QA selectors:

    • file app/assets/javascripts/ci/ci_variable_list/components/ci_variable_drawer.vue:
      •  data-qa-selector="ci_variable_key_field"
      •    data-qa-selector="ci_variable_value_field"

    Please ensure e2e:package-and-test job is run and the tests are passing.

    For the list of known failures please refer to the latest pipeline triage issue.

    If your changes are under a feature flag, please check our Testing with feature flags documentation for instructions.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    backend Sylvester Chin current availability (@schin1) (UTC+8, same timezone as @mgandres) Heinrich Lee Yu current availability (@engwan) (UTC+8, same timezone as @mgandres)
    frontend Thomas Randolph current availability (@thomasrandolph) (UTC-6, 14 hours behind @mgandres) Frédéric Caplette current availability (@f_caplette) (UTC-4, 12 hours behind @mgandres)
    test for spec/features/* Sylvester Chin current availability (@schin1) (UTC+8, same timezone as @mgandres) Maintainer review is optional for test for spec/features/*
    UX Kevin Comoli current availability (@kcomoli) (UTC+2, 6 hours behind @mgandres) Maintainer review is optional for UX

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    Changes to utility SCSS files

    Addition to app/assets/stylesheets/utilities.scss

    You have added a new rule to app/assets/stylesheets/utilities.scss. Are you sure you need this rule?

    If it is a component class shared across items, could it be added to the component as a utility class or to the component's stylesheet? If not, consider adding it to app/assets/stylesheets/framework/common.scss

    If it is a new utility class, is there another class that shares the same values in either this file or in app/assets/stylesheets/utilities.scss? If not, please be sure this addition follows the Gitlab UI naming style so it may be removed when these rules are included. See Include gitlab-ui utility-class library for more about this project.

    If needed, you can retry the :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Contributor

    Bundle size analysis [beta]

    This compares changes in bundle size for entry points between the commits 2d3cdde2 and cd275b15

    :sparkles: Special assets

    Entrypoint / Name Size before Size after Diff Diff in percent
    average 4.15 MB 4.15 MB - 0.0 %
    mainChunk 3.01 MB 3.01 MB - 0.0 %

    :fearful: Significant Growth: 8

    Expand
    Entrypoint / Name Size before Size after Diff Diff in percent
    pages.admin.application_settings 663.14 KB 677.36 KB +14.22 KB 2.1 %
    pages.admin.application_settings.advanced_search 678.63 KB 692.85 KB +14.22 KB 2.1 %
    pages.admin.application_settings.integrations 676.6 KB 690.82 KB +14.22 KB 2.1 %
    pages.admin.application_settings.metrics_and_profiling 667.02 KB 681.24 KB +14.22 KB 2.1 %
    pages.admin.application_settings.network 664.3 KB 678.52 KB +14.22 KB 2.1 %
    pages.admin.application_settings.repository 676.19 KB 690.41 KB +14.22 KB 2.1 %
    pages.admin.application_settings.service_usage_data 666.98 KB 681.2 KB +14.22 KB 2.1 %
    pages.admin.application_settings.templates 687.49 KB 701.71 KB +14.22 KB 2.1 %

    Your MR has at least one entrypoint growing significantly (more > 1 KB or 2%). If you write new or extend existing features, this is expected and there is nothing to worry about.

    Please consider pinging someone from the FE Foundations (@leipert, @markrian, @ohoral or @pgascouvaillancourt) for review, if you are unsure about the size increase.

    Note: We do not have exact data for 2d3cdde2. So we have used data from: 372909ea.
    The target commit was too new, so we used the latest commit from master we have info on.
    It might help to rerun the bundle-size-review job
    This might mean that you have a few false positives in this report. If something unrelated to your code changes is reported, you can check this comparison in order to see if they caused this change.

    Please look at the full report for more details


    Read more about how this report works.

    Generated by :no_entry_sign: Danger

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for cd275b15

    expand test summary
    +------------------------------------------------------------------+
    |                          suites summary                          |
    +-------------+--------+--------+---------+-------+-------+--------+
    |             | passed | failed | skipped | flaky | total | result |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Create      | 19     | 0      | 0       | 18    | 19    | ❗     |
    | Data Stores | 20     | 0      | 0       | 15    | 20    | ❗     |
    | Plan        | 47     | 0      | 0       | 40    | 47    | ❗     |
    | Govern      | 19     | 0      | 0       | 18    | 19    | ❗     |
    | Manage      | 12     | 0      | 1       | 12    | 13    | ❗     |
    | Verify      | 8      | 0      | 0       | 8     | 8     | ❗     |
    +-------------+--------+--------+---------+-------+-------+--------+
    | Total       | 125    | 0      | 1       | 111   | 126   | ❗     |
    +-------------+--------+--------+---------+-------+-------+--------+

    e2e-package-and-test: :x: test report for cd275b15

    expand test summary
    +-------------------------------------------------------------+
    |                       suites summary                        |
    +--------+--------+--------+---------+-------+-------+--------+
    |        | passed | failed | skipped | flaky | total | result |
    +--------+--------+--------+---------+-------+-------+--------+
    | Verify | 95     | 1      | 10      | 2     | 106   | ❌     |
    +--------+--------+--------+---------+-------+-------+--------+
    | Total  | 95     | 1      | 10      | 2     | 106   | ❌     |
    +--------+--------+--------+---------+-------+-------+--------+

    e2e-review-qa: :white_check_mark: test report for cd275b15

    expand test summary
    +-----------------------------------------------------------------------+
    |                            suites summary                             |
    +------------------+--------+--------+---------+-------+-------+--------+
    |                  | passed | failed | skipped | flaky | total | result |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Plan             | 3      | 0      | 1       | 0     | 4     | ✅     |
    | Verify           | 8      | 0      | 0       | 0     | 8     | ✅     |
    | Create           | 8      | 0      | 1       | 0     | 9     | ✅     |
    | Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
    | Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
    | Manage           | 1      | 0      | 0       | 0     | 1     | ✅     |
    | Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
    | Data Stores      | 2      | 0      | 0       | 0     | 2     | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
    | Total            | 28     | 0      | 3       | 0     | 31    | ✅     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • mentioned in merge request gitlab-ui!3569 (merged)

  • Mireya Andres added 316 commits

    added 316 commits

    Compare with previous version

  • Mireya Andres marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • Mireya Andres changed the description

    changed the description

  • Mireya Andres added 188 commits

    added 188 commits

    Compare with previous version

  • 🤖 GitLab Bot 🤖 resolved all threads

    resolved all threads

  • Mireya Andres changed the description

    changed the description

  • Mireya Andres added 2 commits

    added 2 commits

    • ec3e6eb5 - Add feature flag for CI variables drawer
    • cf52c267 - Add other form elements

    Compare with previous version

  • requested review from @veethika, @djadmin, and @rutgerwessels

  • Rutger Wessels approved this merge request

    approved this merge request

  • Rutger Wessels requested review from @jessieay and removed review request for @rutgerwessels

    requested review from @jessieay and removed review request for @rutgerwessels

  • :wave: @rutgerwessels, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • Veethika Mishra approved this merge request

    approved this merge request

  • added UX label

  • Veethika Mishra removed review request for @veethika

    removed review request for @veethika

  • Jessie Young removed review request for @jessieay

    removed review request for @jessieay

  • Dheeraj Joshi
    • Resolved by Dheeraj Joshi

      question: have we considered re-using the same form underneath the modal component for the drawer component?

      1. ci_variable_drawer.vue
      2. ci_variable_modal.vue

      This way we won't have to maintain two separate components with the same form elements.

  • Mireya Andres added 277 commits

    added 277 commits

    Compare with previous version

  • Mireya Andres added 164 commits

    added 164 commits

    Compare with previous version

  • Mireya Andres marked this merge request as ready

    marked this merge request as ready

  • Mireya Andres added 6 commits

    added 6 commits

    Compare with previous version

  • Mireya Andres requested review from @jessieay and @jmiocene

    requested review from @jessieay and @jmiocene

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading