-
Notifications
You must be signed in to change notification settings - Fork 87
feat: publish altinn app service resource to resource registry #17642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: publish altinn app service resource to resource registry #17642
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #17642 +/- ##
==========================================
- Coverage 95.98% 95.92% -0.06%
==========================================
Files 2433 2450 +17
Lines 30850 31209 +359
Branches 3598 3639 +41
==========================================
+ Hits 29612 29938 +326
- Misses 923 957 +34
+ Partials 315 314 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e25919d to
5f8d3ce
Compare
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
editingContext
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to fix a “useless assignment to local variable” where you still need any side effects of the right-hand expression, you remove the local variable and keep only the expression or adjust the code to use the value meaningfully. If the value is not needed and there are no side effects, you remove the entire statement.
In this specific test method (UndeployAsync_WithGitOpsFeatureDisabled_ShouldUseDecommissionDefinitionId in DeploymentServiceTest.cs), the editingContext variable is assigned but never used. To avoid changing behavior while removing the useless assignment, we should keep the AltinnRepoEditingContext.FromOrgRepoDeveloper(...) call as a standalone expression statement and drop the var editingContext = part. This ensures any potential side effects of FromOrgRepoDeveloper are preserved while eliminating the unread local variable. No additional imports, methods, or definitions are needed, and only the single line 828 needs to be adjusted.
-
Copy modified line R828
| @@ -825,7 +825,7 @@ | ||
| ) | ||
| { | ||
| // Arrange | ||
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" |
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
editingContext
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to fix a "useless assignment to local variable" issue, either remove the unused local variable and its assignment if the value is not needed, or start using the variable where appropriate if it was intended to be used. If the right-hand side of the assignment has important side effects, you keep the call but drop only the unused variable.
In this specific test method UndeployAsync_WithGitOpsFeatureEnabled_AppExistsInGitOps_ShouldRemoveAppAndUseGitOpsManagerDefinitionId, the editingContext variable is created but never read. The simplest, behavior‑preserving fix is to remove the variable declaration/assignment entirely. Since the subsequent mock setups use It.IsAny<...>() and there is no later use of editingContext in the snippet, deleting the line that declares it is safe. No additional imports, methods, or definitions are needed; we only remove the unnecessary line.
Concretely, in src/Designer/backend/tests/Designer.Tests/Services/DeploymentServiceTest.cs, delete the line:
var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper(
org,
app,
"testUser"
);and leave the rest of the Arrange section unchanged.
| @@ -947,11 +947,6 @@ | ||
| ) | ||
| { | ||
| // Arrange | ||
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); | ||
|
|
||
| _featureManager | ||
| .Setup(fm => fm.IsEnabledAsync(StudioFeatureFlags.GitOpsDeploy)) |
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
editingContext
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to fix a “useless assignment to local variable” you either (a) remove the variable and, if needed, keep only the side‑effecting call, or (b) start using the variable where it was originally intended to be used. For tests, it’s usually clearer to remove unused locals unless they are clearly needed for assertions or method calls.
For this specific test method UndeployAsync_WithGitOpsFeatureEnabled_AppDoesNotExistInGitOps_NotDeployedInCluster_ShouldUseDecommissionDefinitionId, the simplest fix that does not change existing functionality is to remove the unused variable editingContext while still invoking AltinnRepoEditingContext.FromOrgRepoDeveloper if that call is desired for side effects. If there is no need to call it at all, we can safely remove the entire statement. Since there is no evidence in the shown snippet that any side effects from FromOrgRepoDeveloper are required, the best minimal fix is to delete the line that declares and assigns editingContext.
Concretely, in src/Designer/backend/tests/Designer.Tests/Services/DeploymentServiceTest.cs, within the body of the UndeployAsync_WithGitOpsFeatureEnabled_AppDoesNotExistInGitOps_NotDeployedInCluster_ShouldUseDecommissionDefinitionId test, remove the line:
var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper(
org,
app,
"testUser"
);and not replace it with anything. No additional imports, methods, or definitions are required.
| @@ -1110,11 +1110,6 @@ | ||
| ) | ||
| { | ||
| // Arrange | ||
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); | ||
|
|
||
| _featureManager | ||
| .Setup(fm => fm.IsEnabledAsync(StudioFeatureFlags.GitOpsDeploy)) |
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning test
editingContext
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to fix a “useless assignment to local variable” issue, you either (1) remove the unused variable and its assignment if they are genuinely unnecessary, or (2) start using the variable where it was intended to be used if the lack of usage is the real bug. You must also consider whether the right-hand side expression has side effects and, if so, whether those side effects are required.
For this specific test method UndeployAsync_WithGitOpsFeatureEnabled_AppDoesNotExistInGitOps_ButDeployedInCluster_ShouldUseGitOpsManagerDefinitionId, the variable editingContext is created but never referenced later in the method. The rest of the setup uses mocks on _featureManager, _gitOpsConfigurationManager, _runtimeGatewayClient, _deploymentRepository, and _azureDevOpsBuildClient. There is no indication that editingContext is needed. The cleanest and least invasive fix is to remove the declaration/assignment of editingContext entirely. No additional imports, methods, or definitions are required.
Concretely, in src/Designer/backend/tests/Designer.Tests/Services/DeploymentServiceTest.cs, within that test method, delete the block that declares var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper(org, app, "testUser");, leaving the rest of the Arrange section untouched.
| @@ -1283,11 +1283,6 @@ | ||
| ) | ||
| { | ||
| // Arrange | ||
| var editingContext = AltinnRepoEditingContext.FromOrgRepoDeveloper( | ||
| org, | ||
| app, | ||
| "testUser" | ||
| ); | ||
|
|
||
| _featureManager | ||
| .Setup(fm => fm.IsEnabledAsync(StudioFeatureFlags.GitOpsDeploy)) |
| string path = Path.Combine( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", | ||
| "..", | ||
| "_TestData", | ||
| "ReleasesCollection", | ||
| filename | ||
| ); |
Check notice
Code scanning / CodeQL
Call to 'System.IO.Path.Combine' may silently drop its earlier arguments Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to avoid Path.Combine silently discarding earlier arguments when later arguments may be absolute, use Path.Join instead. Path.Join always concatenates the provided segments with the appropriate directory separator without treating any segment as overriding the prior ones.
For this file, the best minimal fix is to replace the two Path.Combine calls that include the filename parameter with Path.Join. Both methods are test helpers constructing paths into the _TestData folder, and changing to Path.Join will preserve the intended root (unitTestFolder/../../../_TestData/...) even if filename is absolute. No additional using directives are needed because Path.Join is also in System.IO, which is already imported at the top of the file. Behavior for all existing relative filenames remains the same.
Concretely:
- In
GetReleases, replace thePath.Combine(...)call assigningpathwith the same arguments passed toPath.Join(...). - In
GetDeployments, do the same replacement for itspathconstruction.
No other logic changes are required.
-
Copy modified line R739 -
Copy modified line R762
| @@ -736,7 +736,7 @@ | ||
| string unitTestFolder = Path.GetDirectoryName( | ||
| new Uri(typeof(DeploymentServiceTest).Assembly.Location).LocalPath | ||
| ); | ||
| string path = Path.Combine( | ||
| string path = Path.Join( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", | ||
| @@ -759,7 +759,7 @@ | ||
| string unitTestFolder = Path.GetDirectoryName( | ||
| new Uri(typeof(DeploymentServiceTest).Assembly.Location).LocalPath | ||
| ); | ||
| string path = Path.Combine( | ||
| string path = Path.Join( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", |
| string path = Path.Combine( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", | ||
| "..", | ||
| "_TestData", | ||
| "Deployments", | ||
| filename | ||
| ); |
Check notice
Code scanning / CodeQL
Call to 'System.IO.Path.Combine' may silently drop its earlier arguments Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
General approach: Replace the use of Path.Combine with Path.Join when combining path segments that are all intended to be relative parts under a known base directory. Path.Join does not discard earlier components when later arguments are absolute; instead, it concatenates path segments more predictably and avoids the silent truncation issue that Path.Combine has.
Best fix for this file: In each of the three helper methods GetReleases, GetDeployments, and GetEnvironments, change the construction of path from Path.Combine(...) to Path.Join(...), keeping all arguments identical. This preserves the intended relative-layout logic and behavior while eliminating the risk of earlier arguments being dropped if a later segment ever becomes absolute. No changes are needed to imports, since Path.Join is in the same System.IO.Path class already being used via using System.IO;.
Concrete changes:
- In
GetReleases(around line 739–747), replacestring path = Path.Combine(withstring path = Path.Join(. - In
GetDeployments(around line 762–770), replacestring path = Path.Combine(withstring path = Path.Join((this is the line CodeQL flagged). - In
GetEnvironments(around line 785–795), replacestring path = Path.Combine(withstring path = Path.Join(.
No additional methods, imports, or definitions are needed.
-
Copy modified line R739 -
Copy modified line R762 -
Copy modified line R785
| @@ -736,7 +736,7 @@ | ||
| string unitTestFolder = Path.GetDirectoryName( | ||
| new Uri(typeof(DeploymentServiceTest).Assembly.Location).LocalPath | ||
| ); | ||
| string path = Path.Combine( | ||
| string path = Path.Join( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", | ||
| @@ -759,7 +759,7 @@ | ||
| string unitTestFolder = Path.GetDirectoryName( | ||
| new Uri(typeof(DeploymentServiceTest).Assembly.Location).LocalPath | ||
| ); | ||
| string path = Path.Combine( | ||
| string path = Path.Join( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", | ||
| @@ -782,7 +782,7 @@ | ||
| string unitTestFolder = Path.GetDirectoryName( | ||
| new Uri(typeof(DeploymentServiceTest).Assembly.Location).LocalPath | ||
| ); | ||
| string path = Path.Combine( | ||
| string path = Path.Join( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", |
| string path = Path.Combine( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", | ||
| "..", | ||
| "..", | ||
| "..", | ||
| "..", | ||
| "development", | ||
| "azure-devops-mock", | ||
| filename | ||
| ); |
Check notice
Code scanning / CodeQL
Call to 'System.IO.Path.Combine' may silently drop its earlier arguments Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to avoid Path.Combine silently dropping earlier segments when a later argument is an absolute path, you should either validate that all later arguments are relative, or use Path.Join, which concatenates path segments without treating absolute segments as overriding earlier ones.
For this code, the best fix without changing intended functionality is to replace the Path.Combine call in GetEnvironments with Path.Join, mirroring the recommendation and ensuring that even if filename is absolute, it will just be appended as text under the intended base directory. This leaves the rest of the method behavior unchanged. Only one method (GetEnvironments) needs modification; the other two Path.Combine usages (GetReleases and GetDeployments) are structurally the same but were not flagged here, and per instructions we should only directly address the highlighted occurrence.
Path.Join is available in System.IO, which is already imported at the top of the file, so no new imports or dependencies are needed. The change is localized to the lines where path is assigned in GetEnvironments.
-
Copy modified line R785
| @@ -782,7 +782,7 @@ | ||
| string unitTestFolder = Path.GetDirectoryName( | ||
| new Uri(typeof(DeploymentServiceTest).Assembly.Location).LocalPath | ||
| ); | ||
| string path = Path.Combine( | ||
| string path = Path.Join( | ||
| unitTestFolder, | ||
| "..", | ||
| "..", |
5f8d3ce to
fa8b3bb
Compare
| if (publishResponse is ObjectResult objectResult && objectResult.StatusCode == 400) | ||
| { | ||
| updateAuthPolicyTask, | ||
| updateTextResources | ||
| }); | ||
| if (objectResult.Value is ValidationProblemDetails validationProblemDetails) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Publishing service resource failed for {org}/{app}/{shortCommitId}: {JsonSerializer.Serialize(validationProblemDetails.Errors)}" | ||
| ); | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Nested 'if' statements can be combined Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 3 hours ago
In general, to fix this kind of issue, when an inner if is nested inside an outer if and neither has an else branch, you can combine the two conditions into a single if using &&, ensuring you use parentheses when needed to maintain correct precedence and readability.
Concretely in ApplicationInformationService.cs, you should replace the nested if starting at line 152:
if (objectResult.Value is ValidationProblemDetails validationProblemDetails)
{
throw new InvalidOperationException(
$"Publishing service resource failed for {org}/{app}/{shortCommitId}: {JsonSerializer.Serialize(validationProblemDetails.Errors)}"
);
}with a single combined if that incorporates both the outer and inner conditions:
if (publishResponse is ObjectResult objectResult
&& objectResult.StatusCode == 400
&& objectResult.Value is ValidationProblemDetails validationProblemDetails)
{
throw new InvalidOperationException(
$"Publishing service resource failed for {org}/{app}/{shortCommitId}: {JsonSerializer.Serialize(validationProblemDetails.Errors)}"
);
}and remove the now-redundant outer if block that only wrapped the inner if. This keeps the behavior identical: the exception is thrown only when publishResponse is an ObjectResult with StatusCode == 400 and a Value of type ValidationProblemDetails. No new methods or imports are required; we solely restructure the existing conditions.
-
Copy modified lines R150-R154 -
Copy modified lines R156-R158
| @@ -147,14 +147,15 @@ | ||
| $"Publishing service resource failed for {org}/{app}/{shortCommitId}: {contentResult.Content}" | ||
| ); | ||
| } | ||
| if (publishResponse is ObjectResult objectResult && objectResult.StatusCode == 400) | ||
| if ( | ||
| publishResponse is ObjectResult objectResult | ||
| && objectResult.StatusCode == 400 | ||
| && objectResult.Value is ValidationProblemDetails validationProblemDetails | ||
| ) | ||
| { | ||
| if (objectResult.Value is ValidationProblemDetails validationProblemDetails) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Publishing service resource failed for {org}/{app}/{shortCommitId}: {JsonSerializer.Serialize(validationProblemDetails.Errors)}" | ||
| ); | ||
| } | ||
| throw new InvalidOperationException( | ||
| $"Publishing service resource failed for {org}/{app}/{shortCommitId}: {JsonSerializer.Serialize(validationProblemDetails.Errors)}" | ||
| ); | ||
| } | ||
| throw new InvalidOperationException( | ||
| $"Publishing service resource failed for {org}/{app}/{shortCommitId}" |
Description
Verification