Fix bug introduced by !150839

changed milestone to %17.0

added citemplates devopssecure groupstatic analysis sectionsec typebug workflowready for review labels

requested review from @cmiskell

mentioned in merge request !150839 (merged)

	2 Warnings
	This merge request adds or changes templates, please consider updating the corresponding Gitlab Component.
	You've made some app changes, but didn't add any tests. That's OK as long as you're refactoring existing code, but please consider adding any of the maintenancepipelines, maintenancerefactor, maintenanceworkflow, documentation, QA labels.

	1 Message
	This merge request adds or changes files that require a review from the CI/CD Templates maintainers.

This merge request requires a CI/CD Template review. To make sure these changes are reviewed, take the following steps:

Ensure the merge request has the citemplates label. If the merge request modifies CI/CD Template files, Danger will do this for you.
Prepare your MR for a CI/CD Template review according to the template development guide.
Assign and @ mention the CI/CD Template reviewer suggested by Reviewer Roulette.

The following files require a review from the CI/CD Templates maintainers:

lib/gitlab/ci/templates/Jobs/SAST.latest.gitlab-ci.yml

Reviewer roulette

Category	Reviewer	Maintainer
citemplates	`@timofurrer` (UTC+2)	`@avielle` (UTC+2)
~"Verify"	Reviewer review is optional for ~"Verify"	`@djadmin` (UTC+5.5)

Please check reviewer's status!

Reviewer is available!
Reviewer is unavailable!

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

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

Generated by Danger

added maintenanceremoval label

added typemaintenance label and removed typebug label

added 1 commit

7bce91fd - Include template in deprecated flafinder-sast job

Compare with previous version

requested review from @theoretick

@theoretick Can you take another look?

mentioned in merge request gitlab-com/gl-infra/common-ci-tasks!553 (merged)

mentioned in commit gitlab-com/gl-infra/platform/runway/runwayctl@d59c2b53

mentioned in merge request !151123 (closed)

In !151123 (closed) @lpshkil created the exact same change for this as a community contribution. I wanted to call this out, thanks @lpshkil!

changed the description

mentioned in issue #450799 (closed)

mentioned in commit 82edabbe

This is critical for us, we are unable to deploy without changes to our pipelines for every repo.

cc @shoyle1

mentioned in issue #458532 (closed)

approved this merge request

added pipeline:mr-approved label

@theoretick, thanks for approving this merge request.

This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break master, a new pipeline will be started shortly.

Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.

resolved all threads

enabled an automatic merge when all merge checks for 7bce91fd pass

@jleasure Some end-to-end (E2E) tests should run based on the stage label.

Please start the manual:e2e-test-pipeline-generate job in the prepare stage and wait for the tests in the follow-up:e2e:package-and-test-ee pipeline to pass before merging this MR. Do not use Auto-merge, unless these tests have already completed successfully, because a failure in these tests do not block the auto-merge. (E2E tests are computationally intensive and don't run automatically for every push/rebase, so we ask you to run this job manually at least once.)

To run all E2E tests, apply the pipeline:run-all-e2e label and run a new pipeline.

E2E test jobs are allowed to fail due to flakiness. See current failures at the latest pipeline triage issue.

Once done, apply the emoji on this comment.

Team members only: for any questions or help, reach out on the internal #test-platform Slack channel.

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for 7bce91fd

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Package     | 19     | 0      | 12      | 0     | 31    | ✅     |
| Govern      | 66     | 0      | 0       | 0     | 66    | ✅     |
| Data Stores | 27     | 0      | 4       | 0     | 31    | ✅     |
| Create      | 96     | 0      | 9       | 0     | 105   | ✅     |
| Plan        | 51     | 0      | 2       | 0     | 53    | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Verify      | 35     | 0      | 1       | 0     | 36    | ✅     |
| Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
| Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
| Monitor     | 7      | 0      | 0       | 0     | 7     | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 308    | 0      | 29      | 0     | 337   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-package-and-test: test report for 7bce91fd

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Secure      | 9      | 0      | 3       | 0     | 12    | ✅     |
| Create      | 182    | 0      | 21      | 0     | 203   | ✅     |
| Verify      | 18     | 0      | 0       | 0     | 18    | ✅     |
| Govern      | 28     | 0      | 0       | 0     | 28    | ✅     |
| Plan        | 44     | 0      | 4       | 0     | 48    | ✅     |
| Data Stores | 14     | 0      | 8       | 0     | 22    | ✅     |
| Release     | 2      | 0      | 0       | 0     | 2     | ✅     |
| Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
| Package     | 6      | 0      | 8       | 0     | 14    | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 311    | 0      | 44      | 0     | 355   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

thx for that MR, blocking all my pipes

resolved all threads

merged

mentioned in commit 47c05505

added Pick into auto-deploy priority2 severity2 labels

Successfully picked into 17-0-auto-deploy-2024042918.

This merge request will receive additional notifications as it's deployed. You can also use the following chatops command to check its status: /chatops run auto_deploy status https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151298

@twoodham Friendly reminder the Pick into auto-deploy label is reserved for high-priority merge requests required to solve production incidents. All the other merge requests should stick to the auto-deploy schedule https://handbook.gitlab.com/handbook/engineering/deployments-and-releases/deployments/ . Thanks!

@mayra-cabrera This issue was linked to significant pipeline failures across internal projects after introducing this regression and considered urgent. I would consider it applicable to either of the examples used under GitLab.com pick label:

Resolves or mitigates a severity::1/severity::2 incident

Urgent performance or availability fix that can improve the stability of GitLab.com

Could you speak a bit more to why this doesn't seem to warrant the use of the label?

Actually, we did not declare an official incident so I suppose it does not fit the first definition, however the linked bug and discussion classifies it as a severity1

@theoretick

Actually, we did not declare an official incident so I suppose it does not fit the first definition, however the linked bug and discussion classifies it as a severity1.

Please open up a production incident if the availability or operational status of GitLab.com is impacted. This allows other folks (like release managers, the Engineer on-call, and support) to be aware and ahead of this problem.

This issue was linked to significant pipeline failures across internal projects after introducing this regression and considered urgent

I feel the above is mixing the definition of urgent from two different angles: Production Incidents and bugs fixes, incidents are events to mitigate service outages or restore operational statuses, while bug fixes are fixes for broken features. Granted, the impact of bug fixes can lead to production incidents, where the impact (customers affected, performance degration, etc) can be measured and a severity assigned based on it.

Could you speak a bit more to why this doesn't seem to warrant the use of the label?

The customer impact for .com and self-managed is not clear from #458532 (closed), and without awareness of this issue, it was surprising to see an out-of-schedule merge request being deployed, an incident for this one was probably missed.

@mayra-cabrera apologies. I pushed for this fix to be addressed as a high priority (#458532 (comment 1884758319)), but should have opened an incident for this to drive the process.

The customer impact for .com and self-managed is not clear from #458532 (closed), and without awareness of this issue, it was surprising to see an out-of-schedule merge request being deployed, an incident for this one was probably missed.

Many Platform teams were affected by this issue, which led to all pipelines failing with invalid yaml. Additionally, there were multiple reports from customers (see #458532 (comment 1884992460), !151298 (comment 1885697134), !150839 (comment 1885023896), plus a community contributed fix !151123 (closed)). Any project without a test stage which uses SAST would fail with the invalid yaml error.

@mayra-cabrera, @andrewn - apologies for creating a bit a chaos around the process by which we proceeded on this particular MR. Thank you for your kindness and working with us. We'll use this as a teaching moment within devopssecure so that we're aligned with the process and know what to do if/when this happens in the future.

I pushed for this fix to be addressed as a high priority ( #458532 (comment 1884758319)), but should have opened an incident for this to drive the process.
...
so that we're aligned with the process and know what to do if/when this happens in the future.

I'm happy to takeaway that declaring an incident would be the most apt way to handle this situation, however I feel that this discussion feels misaligned with the guidance on the label. The current guidance suggests appropriate use including regressions causing a severity2, which applies to the linked issue (but most likely could be more clearly described I expect). The current guidance is:

The label should be only used under the following circumstances, when the merge request is especially urgent. For example:

Resolves or mitigates a severity::1/severity::2 incident

Resolves a regression that can lead to a severity::1/severity::2 problem

Urgent performance or availability fix that can improve the stability of GitLab.com

@mayra-cabrera Perhaps we should update the guidance to require use of the label only with declared incidents?

@theoretick

The current guidance suggests appropriate use including regressions causing a severity2, which applies to the linked issue (but most likely could be more clearly described I expect).

I don't feel this is accurate. This merge request wasn't part of an incident and can't be classified as a performance fix, as it doesn't prevent GitLab degradation and outages.

This is a high-severity bug that should have been treated as an incident, and on that case, the guidance on the label would apply.

@mayra-cabrera, would be be able to you propose a clarification to the handbook?

Just yesterday I relied on the same content that @theoretick shared to use Pick into auto-deploy for an S2 bug without an incident.

Thanks for reaching out @thiagocsf. Do you mind sharing which merge request you are referring to?

@mayra-cabrera: !152319 (closed), which was later replaced by !152342 (merged).

@thiagocsf Thanks for the details. Given the impact, do you know why an incident wasn't opened in that case?

@mayra-cabrera, I don't know. By the time I heard about it, I focused on getting the revert MR through. I didn't even consider raising an incident.

Retrospecting now, an incident should have been created. With this one, we have 2 recent high-severity bugs that should have been treated as an incident but weren't. Do we have a training or knowledge gap?

It might be useful to ask the engineers involved in the initial thread about their thought process as well. (@mfanGitLab, @fcatteau, @hacks4oats)