Rails: Introduce/support kubernetes error type in reconciliation API

Issue: #428624 (closed)

What does this MR do and why?

• Adds support for new error type kubernetes
• Update relevant specs to verify support

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before	After

How to set up and validate locally

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

MR acceptance checklist

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

• I have evaluated the MR acceptance checklist for this MR.

changed milestone to %16.6

assigned to @hkhanna2

changed title from Draft: Rails: Introduce/support error type in reconciliation API to Draft: Rails: Introduce/support kubernetes error type in reconciliation API

added 1 commit

• 6ff3de5e - Add spec for new error type

Compare with previous version

added backend label

Proper labels assigned to this merge request. Please ignore me.

@hkhanna2 - please see the following guidance and update this merge request.

	1 Error
	Please add typebug typefeature, or typemaintenance label to this merge request.

Allure report

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for 794b97b1

expand test summary

+-----------------------------------------------------------------------+
|                            suites summary                             |
+------------------+--------+--------+---------+-------+-------+--------+
|                  | passed | failed | skipped | flaky | total | result |
+------------------+--------+--------+---------+-------+-------+--------+
| Verify           | 32     | 0      | 0       | 0     | 32    | ✅     |
| Govern           | 48     | 0      | 0       | 0     | 48    | ✅     |
| Create           | 40     | 0      | 6       | 0     | 46    | ✅     |
| Plan             | 55     | 0      | 0       | 0     | 55    | ✅     |
| Data Stores      | 22     | 0      | 0       | 0     | 22    | ✅     |
| Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
| Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
| Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
| Manage           | 0      | 0      | 1       | 0     | 1     | ➖     |
+------------------+--------+--------+---------+-------+-------+--------+
| Total            | 201    | 0      | 9       | 0     | 210   | ✅     |
+------------------+--------+--------+---------+-------+-------+--------+

mentioned in issue #428624 (closed)

changed the description

added Category:Remote Development featureenhancement typefeature labels

resolved all threads

@cwoolley-gitlab can you help with reviewing these changes? However, let's hold off on the merge until the companion MR in agent has finished reviewing?

requested review from @cwoolley-gitlab

Setting label(s) devopscreate groupide sectiondev based on Category:Remote Development groupide.

added devopscreate groupide sectiondev labels

@hkhanna2 praise thanks for drying up the specs some.

Here's a patch that cleans up the spec even more.

The general idea behind this refactor is "don't declare fixtures in scopes where they are not used", which is a good general idea, but not a hard rule if it makes things convoluted/complex. But in this case we can do it and it's cleaner and more expressive, IMO.

The main changes in this are:

1. Don't implicitly test the "applier" case as the default value for error_type, remove the default and push it down only to the contexts where it's needed/used.
2. Introduce a context under the valid case to be explicit about the "applier" type. This helps the tests serve as documentation.
3. In the invalid contexts, we only need to specify error_type where it's needed/used. In one of them we can now just override error_type instead of the whole error details.
4. Use the constants for the values rather than hardcoding them. Type safety is good, it will show errors in the tests if they are changed. Now if we could only have enum types ;)

Index: ee/spec/lib/remote_development/workspaces/reconcile/input/params_validator_spec.rb
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ee/spec/lib/remote_development/workspaces/reconcile/input/params_validator_spec.rb b/ee/spec/lib/remote_development/workspaces/reconcile/input/params_validator_spec.rb
--- a/ee/spec/lib/remote_development/workspaces/reconcile/input/params_validator_spec.rb	(revision 6ff3de5ee961f7fdd154d8242bf0522fe8f04311)
+++ b/ee/spec/lib/remote_development/workspaces/reconcile/input/params_validator_spec.rb	(date 1697693080099)
@@ -6,7 +6,6 @@
   include ResultMatchers
 
   let(:update_type) { "full" }
-  let(:error_type) { "applier" }
   let(:workspace_error_details) do
     {
       error_type: error_type,
@@ -41,16 +40,20 @@
       end
     end
 
-    it_behaves_like 'success result'
-
     context 'when error_details nil' do
       let(:workspace_error_details) { nil }
 
+      it_behaves_like 'success result'
+    end
+
+    context 'when error_type is "applier"' do
+      let(:error_type) { RemoteDevelopment::Workspaces::Reconcile::ErrorType::APPLIER }
+
       it_behaves_like 'success result'
     end
 
     context 'when error_type is "kubernetes"' do
-      let(:error_type) { "kubernetes" }
+      let(:error_type) { RemoteDevelopment::Workspaces::Reconcile::ErrorType::KUBERNETES }
 
       it_behaves_like 'success result'
     end
@@ -85,6 +88,7 @@
 
     context 'for update_type' do
       context 'when not "partial" or "full"' do
+        let(:error_type) { RemoteDevelopment::Workspaces::Reconcile::ErrorType::KUBERNETES }
         let(:update_type) { "INVALID UPDATE TYPE" }
 
         it_behaves_like 'err result', expected_error_details:
@@ -105,12 +109,7 @@
       end
 
       context 'when error_type has an invalid value' do
-        let(:workspace_error_details) do
-          {
-            error_type: "unknown",
-            error_message: "something has gone wrong"
-          }
-        end
+        let(:error_type) { 'invalid error type' }
 
         it_behaves_like 'err result', expected_error_details:
           %(property '/workspace_agent_infos/0/error_details/error_type' is not one of: ["applier", "kubernetes"])

diff.patch

marked this merge request as ready

added 1 commit

• bdb56e0d - Refactor spec

Compare with previous version

added 699 commits

• bdb56e0d...01defe9d - 698 commits from branch master
• a4fe674c - Introduce KUBERNETES error type

Compare with previous version

added 316 commits

• a4fe674c...c7047267 - 315 commits from branch master
• 3cbad139 - Introduce KUBERNETES error type

Compare with previous version