separate dev and prod env setup #92

JudeNiroshan · 2025-10-22T08:06:38Z

Introduce the deployment setup for individual envs. dev & prod.

Dedicated make commands and dedicated argocd application sync are not created for these 2 envs

Signed-off-by: Jude Niroshan <[email protected]>

github-actions

Okay, I am ready. Let me know when you are ready.

Okay, I've reviewed the changes and here's a summary of the potential issues and suggestions for improvement:

Review Summary:

The changes seem primarily focused on streamlining the image tagging and updating versions in the release workflow. The removal of the latest-dev tag in the dev workflow and the shift to updating values-prod.yaml instead of values.yaml in the release workflow are the key changes. While the changes appear logical, there are some points to consider regarding clarity, rollback strategy, and documentation.

File-Specific Comments:

File: .github/workflows/build-dev-image.yml
- Issue: The removal of latest-dev tag and using latest for dev image might cause confusion. It's common to distinguish between stable latest and potentially unstable latest-dev.
- Suggestion: Add a comment explaining why latest-dev was removed and latest is now used for dev. Consider if latest tag overwrites the tagged release. Also, state how latest is managed for development and production environments.
File: .github/workflows/build-release-image.yml
- Issue: Updating values-prod.yaml directly. This implies different configurations for production and potentially other environments.
- Suggestion: Add a comment explaining why values-prod.yaml is specifically targeted. Consider a more robust templating solution (like yq) if the differences between environments become more complex. Also, consider if other environments should be supported.
- Issue: The sed command updating the Helm chart version could be more robust. If the version: line in Chart.yaml has leading whitespace, the sed command will fail.
- Suggestion: Use sed -i "s/^ *version: .*/version: $POM_VERSION/" deploy/sast-ai-chart/Chart.yaml to handle leading whitespace.
- Issue: The commit message "Update versions to ${{ github.event.release.tag_name }}" is now misleading as it only updates the production versions.
- Suggestion: Change the commit message to "Update production versions to ${{ github.event.release.tag_name }}".
- Issue: The documentation update in the release notes mentions Quay.io. While this might be the current registry, hardcoding it makes the documentation less flexible.
- Suggestion: Consider using the env.REGISTRY variable in the documentation, similar to how IMAGE_NAME is used. This makes it easier to switch registries in the future.

In summary, the changes are generally good, but adding comments to explain the reasoning behind key decisions and making the sed command more robust will improve the overall quality and maintainability of the workflows. Also, it is important to consider a well-defined strategy for managing multiple environments (dev, staging, prod, etc.) and how configuration changes are handled across them.

Okay, I've reviewed the changes. Here's a summary of the issues and suggestions:

Review Summary:

The changes focus on improving the deployment documentation and process, particularly around differentiating between development and production environments. The updates introduce environment-specific deployment steps, commands, and access instructions. The changes are generally good and improve clarity.

File-Specific Comments:

README.md
- Issue: None major. The added deployment information is well-structured and helpful.
- Suggestion: Consider adding a note about the implications of using latest tag in development (e.g., potential for instability).
deploy/Makefile
- Issue: None.
- Suggestion: Add comments explaining the purpose of each command and variable to enhance readability, especially for those unfamiliar with Makefiles.

Overall, the changes improve the deployment documentation and process. Consider the suggestions to further enhance clarity and user experience.

Okay, here's a review of the Makefile changes, focusing on clarity, simplicity, redundancy, and potential issues:

Review Summary:

The changes introduce separate deploy and upgrade targets for "dev" and "prod" environments. This is generally a good idea for managing different configurations. The introduction of internal _deploy and _upgrade targets reduces duplication. However, there are a few areas where clarity and efficiency could be improved, and potential issues identified.

File: Makefile

Issue Comments:

Redundancy in deploy and upgrade targets:
- The deploy-dev, deploy-prod, upgrade-dev, and upgrade-prod targets all contain duplicated echo commands and then call the internal _deploy or _upgrade target. This is good for re-usability.
- Consider adding these variables PROD_NAMESPACE and DEV_NAMESPACE at the top of the Makefile.
Missing PROD_NAMESPACE and DEV_NAMESPACE Definitions:
- The deploy-prod target uses PROD_NAMESPACE, but it's not defined. Similarly, DEV_NAMESPACE is used but might not be explicitly defined. This should be defined with a default value similar to NAMESPACE. Without defaults, the commands will likely fail.
- NAMESPACE ?= $(if $(CURRENT_NAMESPACE),$(CURRENT_NAMESPACE),$(DEV_NAMESPACE)) is modified. Ensure DEV_NAMESPACE is always defined before this line, or the logic will break.
ArgoCD Application Creation Namespace:
- The argocd target is called without a namespace. It should be called with the appropriate namespace to create the ArgoCD application in the correct environment. The fix is to pass the NAMESPACE variable, just like it's done for wait-pods and show-access.
values.yaml duplication:
- The helm commands in _deploy and _upgrade include -f $(CHART_PATH)/values.yaml and -f $(CHART_PATH)/$(VALUES_FILE). This means values.yaml is always loaded, and then the environment-specific file is loaded afterward, potentially overwriting settings. If the intention is to only load the environment-specific file, remove the first -f argument. If the intention is to load values.yaml as a base and then override with environment-specific settings, this is fine, but should be documented.
Error Handling:
- The helm repo add command includes >/dev/null 2>&1 || true. This suppresses errors, which can hide real problems. Consider removing || true and letting the script fail if the repo add fails. This will make debugging easier. If suppressing the error is truly necessary, add a comment explaining why.
Clarity of KUBECTL_CMD:
- The echo "Using: $(KUBECTL_CMD)" lines are helpful, but it might be even better to include the actual command that will be executed in the echo. This would make debugging much easier. For example: echo "Executing: $(KUBECTL_CMD) get pods -n $(NAMESPACE)". This adds verbosity, but is useful for debugging and traceability, and can be applied to all echo statements before the execution.
wait-pods and show-access Namespace:
- It's good that wait-pods and show-access now receive the NAMESPACE variable. Make sure those targets actually use the NAMESPACE variable internally. I'll need to see those targets to confirm.

Recommendations:

Define PROD_NAMESPACE and DEV_NAMESPACE with default values at the top of the Makefile.
Pass the correct NAMESPACE when calling the argocd target.
Review the use of values.yaml and environment-specific files to ensure the intended configuration loading order. Document the intention.
Reconsider suppressing errors from helm repo add.
Consider adding the full command to echo statements for increased clarity.
Verify that wait-pods and show-access targets properly use the passed NAMESPACE variable.

I'm ready for the next file.

Okay, here's a review of the Makefile changes, focusing on clarity, simplicity, efficiency, and security:

General Summary:

The changes introduce improved cleanup procedures, environment-specific clean commands (dev/prod), and a status command. The cleanup is more aggressive, which is generally good, but also carries a higher risk of unintended deletion if labels or release names are not strictly controlled.

File: Makefile

Clarity and User Experience:
- The change from "Upgrade completed!" to "✅ Upgrade completed!" improves user feedback.
- Consider adding descriptions to the status target.
Organization and Duplication:
- The new _clean target is excellent for reducing duplication. Good job!
Potential Issues:
- The aggressive cleanup in _clean using grep "$(RELEASE_NAME)" is potentially dangerous. If RELEASE_NAME is a substring of another resource's name, it will be deleted. Suggest using more specific label-based selectors.
- The loop in _clean iterates through many resource types. This is generally okay, but consider if some of these resource types are unlikely to exist in most deployments. If so, removing them from the loop could improve performance slightly.
Security Considerations:
- The cleanup process relies on the user's configured KUBECTL_CMD having the appropriate permissions to delete resources in the specified namespaces. Ensure that the principle of least privilege is followed when configuring access to the Kubernetes cluster.

Recommendations:

Makefile: _clean Target - Improve Resource Selection: Instead of grep "$(RELEASE_NAME)", explore ways to make the resource selection more precise. If possible, rely more heavily on labels. If not feasible, consider adding a prefix or suffix to RELEASE_NAME to reduce the risk of accidental deletion.
Makefile: Consider separate KUBECTL_CMD for cleanup: If possible, use a dedicated KUBECTL_CMD that has very restrictive permissions.

Example of suggestion #1:

 #Use this when creating your resources
 MY_APP_NAME=my-very-unique-app

 #Then when cleaning up your resources you can specify it
 @$(KUBECTL_CMD) delete all -n $(NAMESPACE) -l app.kubernetes.io/name=$(MY_APP_NAME) --ignore-not-found=true 2>/dev/null || true

Overall, these are good changes that improve the usability and cleanup of the deployment process. The main area of concern is the potential for over-aggressive cleanup, which needs to be addressed with more precise resource selection.

Okay, I've reviewed the changes. Here's a summary of my findings and some suggestions for improvement.

Review Summary:

The changes introduce environment-specific deployment targets (dev/prod) with corresponding commands, which is good for managing different environments. The addition of a status command is helpful for monitoring deployments. The removal of the hardcoded ArgoCD application and adjustments to the help command improve usability and flexibility.

File-Specific Comments:

File: Makefile

Improvement: Consider using ifndef to define variables only if they are not already defined in the environment. This prevents accidental overriding of environment variables.
Improvement: It would be beneficial to include instructions on how to set the DEV_NAMESPACE and PROD_NAMESPACE variables, perhaps with default values if not explicitly set.
Suggestion: The status command uses 2>/dev/null to suppress errors. Consider adding a more descriptive message when a deployment is not found, like "Deployment not found. Ensure it has been deployed". This would make debugging easier.
Suggestion: Add a check to ensure that helm is installed before running any helm commands. Provide a helpful error message if it's not found.
Security: Consider adding a check to ensure the user is authenticated against the Kubernetes cluster before running any deployment commands.

File: deploy/argocd/application.yaml

Comment: This file has been deleted, implying that ArgoCD deployment will be handled differently. Ensure the new approach is well-documented and tested. Consider how the application was previously deployed to ArgoCD and how it will be deployed now.

File: deploy/sast-ai-chart/values.yaml

Comment: This file now contains shared configuration, which is a good practice. Ensure that environment-specific overrides in values-dev.yaml and values-prod.yaml are clearly defined and well-documented.

Okay, I've reviewed the provided values.yaml file differences. Here's a summary of the issues and suggestions:

Review Summary:

The changes aim to make the Helm chart more configurable and environment-aware, which is a good practice. The shift towards environment-specific configurations makes the chart more flexible and avoids hardcoding values. However, there are opportunities to improve clarity, structure, and security aspects.

File: values.yaml - Issue Comments:

Clarity and Organization:
- The comments "shared" and "can be overridden" are good. Consider adding a short explanation why a section is shared or overridable to guide users. For example, "Shared configuration, applicable across all environments. Environment-specific values will take precedence."
- It might be good to call out what values are mandatory versus optional.
Image Tagging and Pull Policy:
- Good to remove the hardcoded image tag and pullPolicy from the base values.yaml. This forces environment-specific overrides, which is desirable.
Environment Variables:
- Clarify which env vars must be overridden in environment-specific files. For example, the comment "Quarkus configuration (overridden per environment)" could be more specific about which Quarkus configurations. Consider providing defaults if there is a sensible default value (but ensure security implications are considered).
PostgreSQL Configuration:
- The removal of hardcoded PostgreSQL credentials (postgresPassword, username, password) is excellent from a security perspective. It must be clear to users that they must provide these values in environment-specific files or via secrets. Add a comment emphasizing this requirement. Also, make sure the chart templates handle the case where these values are not provided (fail fast with an error).
- Consider using a more robust secret management solution than just specifying an existingSecret. Tools like HashiCorp Vault or cloud-specific secret management services (AWS Secrets Manager, Azure Key Vault, GCP Secret Manager) could be integrated for better security. However, this might be out of scope for this specific PR.
OpenShift Compatibility:
- The comments regarding OpenShift compatibility are good. Ensure that the chart templates correctly handle the podSecurityContext and other OpenShift-specific settings.
Route Host:
- Removing the default host from the route config and allowing OpenShift to manage it is good.
RBAC:
- Ensure the RBAC configuration is as restrictive as possible, granting only the necessary permissions to the SAST AI Orchestrator service account. Review the specific permissions being requested.

General Suggestions:

Documentation: Provide clear documentation on how to configure the chart for different environments. This should include examples of environment-specific values.yaml files and instructions on how to create and manage secrets.
Validation: Implement schema validation for the values.yaml file to catch configuration errors early.
Testing: Thoroughly test the chart in different environments (including OpenShift) to ensure that it works as expected.
Security Audit: Conduct a security audit of the chart to identify and address any potential security vulnerabilities. Pay special attention to secret management, RBAC, and network policies.

Let me know if you want me to elaborate on any of these points or review specific parts of the chart templates.

Okay, I've reviewed the changes you've provided. Here's a summary and some specific comments:

Review Summary:

The changes look generally good and aim to improve the configurability and clarity of the Helm chart and remove an unused dependency. The removal of DvcTaskResults.java suggests a refactoring of how DVC task results are handled.

File-Specific Comments:

charts/sast-controller/values.yaml:
- Clarity: Adding "(shared)" to the configMap description is good for clarity.
- Organization: Grouping disabled features (HPA, PDB, Monitoring, NetworkPolicy) together is a good practice.
- Consider using a more descriptive name for the monitoring section, such as observability, to better reflect its purpose.
pom.xml:
- Dependency Removal: Removing quarkus-flyway is fine, assuming it's no longer needed. Add a comment to the commit message to explain why it's removed.
src/main/java/com/redhat/sast/api/common/constants/DvcTaskResults.java:
- Removal: Removing this file is a significant change.
- Important: You MUST provide context on WHY this file was removed and HOW the functionality it provided is now handled. This is CRITICAL for understanding the change. If the constants are inlined, then it is fine to remove this.

Overall, the changes appear reasonable. However, the removal of DvcTaskResults.java requires a clear explanation of the new implementation.

Okay, I've reviewed the code differences you provided. Here's a summary of the issues and suggestions:

Review Summary:

The code removes EXECUTION_TIMESTAMP from DvcConstants and deletes the entire DvcMetadata class. This suggests a significant change in how DVC metadata is handled. Without context, it's impossible to determine if this is correct, but it warrants careful scrutiny.

File Comments:

File: src/main/java/com/redhat/sast/api/DvcConstants.java
- Issue: Removal of EXECUTION_TIMESTAMP.
- Suggestion:
  - Add a comment explaining why EXECUTION_TIMESTAMP is being removed. What replaces it?
  - Is the value EXECUTION_TIMESTAMP being used anywhere else in the codebase? If so, make sure to remove it or update it accordingly.
File: src/main/java/com/redhat/sast/api/dto/DvcMetadata.java
- Issue: Complete deletion of the DvcMetadata class.
- Suggestion:
  - This is a major change. There should be a clear explanation of why this class is being removed.
  - Where is the information that was previously stored in DvcMetadata now being stored?
  - What impact does this have on other parts of the application that used DvcMetadata?

Without knowing the context and purpose of these changes, it's difficult to provide more specific feedback. However, the removal of a DTO and a constant suggests a potential refactoring or architectural change. Make sure the commit message and pull request description clearly explain the reasoning and impact of these changes.

Okay, I've reviewed the code differences you've provided. Here's a breakdown of the issues and suggestions:

Review Summary:

The changes primarily involve removing a builder class, deleting an exception class, and modifying the DataArtifact and Job entities. The removal of the builder seems reasonable if it wasn't adding significant value. The DataArtifact changes add an auto-generated ID and an index, which are good practices. The removal of DVC related columns from the Job entity suggests a shift in how that data is being handled.

File-Specific Comments:

src/main/java/com/redhat/sast/api/model/DvcMetadata.java
- Issue: Removal of the builder class.
- Suggestion: Ensure that the removal of the builder pattern doesn't negatively impact the creation and maintainability of DvcMetadata objects. Consider if there is another way to construct this object if needed.
src/main/java/com/redhat/sast/api/exceptions/DataArtifactCreationException.java
- Issue: Deletion of the DataArtifactCreationException class.
- Suggestion: Before deleting, ask if this exception is no longer needed or if it is replaced by a generic exception. Ensure that the logic that previously threw this exception now throws a more appropriate and specific exception, or handle the error appropriately.
src/main/java/com/redhat/sast/api/model/DataArtifact.java
- Improvement: Addition of @GeneratedValue(strategy = GenerationType.IDENTITY) and id column.
- Comment: This is a good change. Using GenerationType.IDENTITY is generally a good choice for auto-generating IDs in relational databases.
- Improvement: Addition of @Index annotation.
- Comment: adding index is an excellent improvement.
src/main/java/com/redhat/sast/api/model/Job.java
- Issue: Removal of dvcDataVersion, dvcPipelineStage, and dvcCommitHash columns.
- Suggestion: Understand why these columns were removed. Where is this information now stored? Was it moved to another entity, or is it no longer needed? The removal might indicate a change in how DVC data is being tracked.
- Suggestion: Consider adding indexes to columns frequently used in queries (if any).

Overall, the changes seem reasonable. But more context around the removal of certain features is necessary to fully assess the impact.

--- a/src/main/java/com/redhat/sast/api/platform/PipelineRunWatcher.java
+++ b/src/main/java/com/redhat/sast/api/platform/PipelineRunWatcher.java
@@ -3,8 +3,6 @@
 import java.util.concurrent.CompletableFuture;
 
 import com.redhat.sast.api.enums.JobStatus;
--import com.redhat.sast.api.service.DataArtifactService;
--import com.redhat.sast.api.service.DvcMetadataService;
 import com.redhat.sast.api.service.JobService;
 
 import io.fabric8.knative.pkg.apis.Condition;
@@ -28,41 +24,50 @@ public class PipelineRunWatcher implements Watcher<PipelineRun> {
     private final long jobId;
     private final CompletableFuture<Void> future;
     private final JobService jobService;
--    private final DvcMetadataService dvcMetadataService;
--    private final DataArtifactService dataArtifactService;
 
     public PipelineRunWatcher(
 -            String pipelineRunName,
 -            long jobId,
 -            CompletableFuture<Void> future,
 -            JobService jobService,
--            DvcMetadataService dvcMetadataService,
-            DataArtifactService dataArtifactService) {
+            String pipelineRunName, long jobId, CompletableFuture<Void> future, JobService jobService) {
         this.pipelineRunName = pipelineRunName;
         this.jobId = jobId;
         this.future = future;
         this.jobService = jobService;
--        this.dvcMetadataService = dvcMetadataService;
--        this.dataArtifactService = dataArtifactService;
     }
 
     @Override
-    public void eventReceived(Action action, PipelineRun pipelineRun) {
+    public void eventReceived(Action action, PipelineRun resource) {
         LOGGER.info(
                 "Watcher event: {} for PipelineRun: {}",
                 action,
--                pipelineRun.getMetadata().getName());
+-                resource.getMetadata().getName());
 
--        var pipelineSuccessCondition = findSuccessCondition(pipelineRun);
-        if (pipelineSuccessCondition.isEmpty()) {
-            return;
-        }
-
-        var condition = pipelineSuccessCondition.get();
-        switch (condition.getStatus()) {
-            case STATUS_TRUE -> handleSuccessfulPipeline(pipelineRun);
-            case STATUS_FALSE -> handleFailedPipeline(condition);
-            default -> handleRunningPipeline(condition);
+        if (resource.getStatus() != null && resource.getStatus().getConditions() != null) {
+            for (Condition condition : resource.getStatus().getConditions()) {
+                if (SUCCEEDED_CONDITION.equals(condition.getType())) {
+                    if (STATUS_TRUE.equalsIgnoreCase(condition.getStatus())) {
+                        LOGGER.info("PipelineRun {} succeeded.", pipelineRunName);
+                        jobService.updateJobStatus(jobId, JobStatus.COMPLETED);
+                        future.complete(null);
+                        return;
+                    } else if (STATUS_FALSE.equalsIgnoreCase(condition.getStatus())) {
+                        LOGGER.error(
+                                "PipelineRun {} failed. Reason: {}, Message: {}",
+                                pipelineRunName,
+                                condition.getReason(),
+                                condition.getMessage());
+                        if (!condition.getReason().equalsIgnoreCase("Cancelled")) {
+                            jobService.updateJobStatus(jobId, JobStatus.FAILED);
+                        }
+                        future.complete(null);
+                        return;
+                    } else {
+                        LOGGER.info(
+                                "PipelineRun {} in progress. Status: {}", pipelineRunName, condition.getStatus());
+                    }
+                }
+            }
+        }
     }
+}
\ No newline at end of file

Review Summary:

The code has improved by directly iterating through the conditions and checking for "Succeeded" condition. This approach eliminates the need for a separate findSuccessCondition method and simplifies the logic. Also the DvcMetadataService and DataArtifactService has been removed from the constructor.

File: platform/PipelineRunWatcher.java

Comment: Good job simplifying the condition checking and removing unused services from the constructor. The code is more readable and efficient.

Okay, I've reviewed the changes. Here's a summary and some specific comments:

Review Summary:

The primary change appears to be the removal of several private helper methods related to handling different PipelineRun states (success, failure, running) and extracting/processing data. The logic from these methods seems to have been inlined directly into the eventReceived method.

General Observations:

Readability: While inlining can sometimes be simpler, in this case, it likely makes the eventReceived method significantly longer and harder to read. The original approach of using well-named helper methods improved code clarity.
Maintainability: Having separate methods for handling different states makes the code easier to modify and debug. If the logic for handling a successful pipeline needs to change, it's much easier to find and modify in a dedicated method.
Duplication: Consider if the inlining has introduced any duplicated code blocks.

File: PipelineRunWatcher.java

Issue: Code Clarity and Maintainability. The removal of the helper methods (findSuccessCondition, handleSuccessfulPipeline, handleFailedPipeline, handleRunningPipeline, executeWithErrorHandling) significantly reduces the readability and maintainability of the eventReceived method.
- Suggestion: Reintroduce the helper methods. Keep the eventReceived method focused on the core logic of determining the PipelineRun state and calling the appropriate handler method. This will make the code easier to understand, debug, and modify in the future.
Issue: Potential Duplication. Examine the inlined code carefully for any opportunities to refactor repeated code blocks into new helper methods.
Minor Comment: The change doesn't appear to introduce any immediate security vulnerabilities.

Okay, I've reviewed the code diffs. Here's a summary of potential issues and suggestions:

General Review Summary:

The code removes DataArtifactRepository and DataArtifactService, and related model class DataArtifact. This is a significant change, and without context on why this functionality is being removed, it's difficult to assess the impact. If the artifact tracking/management functionality is no longer needed, then the removal is appropriate. However, if this functionality is being moved or replaced, I'd need to see the replacement code to ensure feature parity and proper integration.

File-Specific Comments:

src/main/java/com/redhat/sast/api/controller/JobController.java
- The processJob method's exception handling is too broad (Exception e). It's better to catch specific exceptions (e.g., JsonProcessingException, IOException, etc.) to handle each case appropriately. This allows for more targeted error recovery and logging.
- The LOGGER.error call within the catch block should include the exception itself (e) so that the full stack trace is logged.
- Consider adding more context to log messages. For example, log the pipeline run name or other relevant details.
src/main/java/com/redhat/sast/api/repository/DataArtifactRepository.java
- This file is being deleted. If the functionality is being moved, ensure the new location is clear and the code is well-tested.
src/main/java/com/redhat/sast/api/service/DataArtifactService.java
- This file is being deleted. Similar to the repository, if the service is being moved, ensure the new location is clear and the code is well-tested.

High-Level Questions/Concerns:

Why are DataArtifactRepository and DataArtifactService being removed? What is the motivation behind this change?
Is there a replacement for the data artifact tracking functionality? If so, where is it?
What is the impact of removing this functionality on other parts of the application?

Without answers to these questions, it's impossible to give a definitive assessment of the pull request. Please provide more context about the purpose of these changes.

Okay, I've reviewed the code you provided. Here's a summary of potential issues and suggestions for improvement:

Review Summary:

The DataArtifactService class seems reasonable for managing data artifacts. However, there are opportunities for improved clarity, potential performance enhancements, and better error handling. The reliance on automatically generated artifact IDs might also be a point of concern if uniqueness isn't guaranteed. The calculateHash() method's implementation is missing, making it difficult to assess its correctness and security implications.

File: DataArtifactService.java

Issue: Missing import statements. The code refers to LOGGER but there is no import statement for it.
- Suggestion: Add import org.slf4j.Logger; and import org.slf4j.LoggerFactory; and replace LOGGER with log
Issue: The class is annotated with @ApplicationScoped. Is this the correct scope? Consider if a more fine-grained scope is needed.
- Suggestion: Review the scoping to ensure it aligns with the application's requirements.
Issue: The @Transactional annotation is applied on methods. This might be too broad.
- Suggestion: Narrow the scope of the transaction to only the database operations, to prevent holding the transaction open for extended periods.
Issue: The calculateHash() method implementation is missing
- Suggestion: Implement or inject the required hash calculation logic. Consider the security implications of the chosen hash algorithm.
Issue: The createBasicArtifactMetadata method is not defined in this code snippet.
- Suggestion: Provide the code for createBasicArtifactMetadata to allow for a complete review.
Issue: The default version is hardcoded as "v1.0.0".
- Suggestion: Consider externalizing this configuration value.
Issue: The path /shared-data/output/sast_ai_output.xlsx is hardcoded.
- Suggestion: Externalize the path to configuration.
Issue: DataArtifactCreationException is used as a generic exception.
- Suggestion: Create more specific exception types to provide better context.

I am ready for the next file diff.

Okay, I've reviewed the code diffs. Here's a summary of the key issues and suggestions for improvement:

Review Summary:

The primary change is the removal of DataArtifactService and DvcMetadataService. This suggests a potentially significant architectural shift. Without knowing the reason for the removal, it's difficult to assess the full impact. However, I can identify potential problems if the functionality of these services isn't properly handled elsewhere. The removed code appears to manage data artifact creation, metadata generation, and DVC integration (tracking data versions, commit hashes, and pipeline stages). It also handles extracting results from Tekton PipelineRuns.

File-Specific Comments:

File: src/main/java/com/redhat/sast/api/service/DataArtifactService.java (Deleted)

Potential Issue: Loss of Data Artifact Management: This service seems responsible for creating and managing data artifacts. Where is this logic moved to? Is it now handled directly within the JobService or elsewhere?
Potential Issue: Duplication: The helper methods (generateArtifactId, calculateHash, createBasicArtifactMetadata) might be needed elsewhere. Ensure these functions are moved to a common utility class or handled differently to avoid duplication.
Clarity: The purpose of calculateHash is unclear. A simple UUID is not a cryptographic hash. If security or data integrity is a concern, a real hashing algorithm should be used. If not, rename it.
Exception Handling: The createDataArtifacts method catches a generic Exception. This is too broad. It should catch specific exceptions and handle them appropriately (e.g., retry, log, fail the job).

File: src/main/java/com/redhat/sast/api/service/DvcMetadataService.java (Deleted)

Potential Issue: Loss of DVC Integration: This service manages the integration with DVC (Data Version Control). Where is this logic moved to? Is data versioning still being tracked?
Potential Issue: Tekton PipelineRun Handling: The updateDvcMetadata method extracts information from Tekton PipelineRuns. How is this information now being extracted and used?
Consider Refactoring Before Deletion: Before deleting this service, determine the function of extractPipelineResults and extractCoreMetadata and then determine where these functions should be moved to.
Missing Context: This service has many helper functions. Without knowing the surrounding context it is hard to determine the full purpose of this code.

General Suggestions:

Explain the Rationale: The PR description must clearly explain why these services are being removed. What problem does this solve? What are the trade-offs?
Address Potential Issues: Explicitly state where the functionality of the removed services has been moved to. Provide code examples if possible.
Avoid Duplication: Ensure that any helper methods or utility functions are moved to a common location to prevent code duplication.
Specific Exception Handling: Use specific exception types and handle them appropriately.
Review Security Implications: Ensure that removing these services doesn't introduce any security vulnerabilities (e.g., loss of data integrity checks, improper handling of sensitive data).
Testing: Add or modify unit and integration tests to ensure the new architecture functions correctly and that no functionality has been lost.
Consider Alternatives: If the intention is to consolidate services, consider refactoring instead of deleting. Could the functionality of DataArtifactService and DvcMetadataService be merged into JobService or another existing service?
Document Changes: Update any relevant documentation to reflect the changes in architecture.

Without more context on why these services are being removed, it's difficult to provide more specific feedback. The key is to ensure that the functionality they provided is handled correctly and efficiently elsewhere in the system. The PR description needs to clearly articulate this.

Okay, I'm ready to review the code differences. Please provide the code snippets. I will focus on clarity, simplicity, efficiency, and security.

Okay, I've reviewed the code and here are my observations and suggestions:

Review Summary:

The code focuses on creating and validating data artifacts based on metadata extracted from a DVC pipeline. It includes validation functions for different field types (commit hash, data version, URL, issue count). The general structure is good, with clear separation of concerns. However, there are opportunities to improve error handling, validation logic, and code clarity. There are also some potential security concerns with URL validation and data version validation (ReDoS).

File: (Assuming this is a single code snippet, please provide the actual file name for accurate referencing)

Here are some issue comments and improvement suggestions:

Clarity and Error Handling:
- validateField: The switch statement's default case could be more explicit. Consider adding a log message to indicate that an unhandled field name was encountered.
- The IllegalStateException is used frequently. While appropriate, consider creating custom exception types (e.g., InvalidCommitHashException, InvalidDataVersionException) to provide more context and allow for more specific exception handling in calling code.
- The catch block with generic Exception should be avoided.
Validation Logic:
- validateUrl: Consider using a well-established URL validation library (e.g., java.net.URL with appropriate try-catch) instead of a simple startsWith check, or consider using regex.
- validateDataVersion: The ReDoS prevention by limiting input length is good. However, consider adding a more robust validation of the content of the data version if possible, perhaps against a known schema or pattern.
Security:
- validateUrl: The startsWith("http://") || value.startsWith("https://") is very basic and could be bypassed. For example, a URL like javascript:alert('xss') would pass this check. Stronger URL validation or sanitization is needed.
Efficiency:
- If fieldName is coming from a limited, known set of values, consider using an enum instead of strings for the fieldName parameter of validateField. This would improve type safety and performance.

I'm ready for the next file differences. Please provide the file name along with the code snippet.

Okay, I've reviewed the code. Here's a summary of the potential issues and suggestions:

Review Summary:

The code primarily focuses on validating and processing DVC metadata, then creating a data artifact. The validation logic is a bit verbose and could be simplified using more modern Java features. The use of regular expressions for validation, while functional, might become a maintenance issue if the requirements change. The artifact creation logic seems sound, but I'd want to confirm the behavior of dataArtifactService.createDataArtifact in a real-world scenario.

File-Specific Comments:

DvcMetadataProcessor.java
- Issue: The validation logic (e.g., validateVersion, validateUrl, validateIssuesCount) is repetitive. There's a lot of duplication in the error message creation and conditional logic based on isRequired.
  
  Suggestion: Refactor the validation methods into a single, more generic validation function that accepts a validation function (e.g., a Predicate), the value to validate, an error message, and the isRequired flag. This would significantly reduce code duplication and improve readability. Consider using Optional for values that might be null or invalid.
- Issue: Regular expressions for URL and version validation can be brittle and hard to maintain.
  
  Suggestion: For URL validation, consider using java.net.URL and catching MalformedURLException. For version validation, investigate using a dedicated semantic versioning library if strict adherence to SemVer is required. If it's not strictly SemVer, consider a more robust parsing approach instead of just relying on a regex.
- Issue: The buildMetadataMap method uses Stream.of(entry(...)) which is okay, but slightly less readable than a standard map initialization, especially if you're only ever adding these specific entries.
  
  Suggestion: Consider a more direct map initialization:
```
Map<String, Object> metadata = new HashMap<>();
metadata.put("analysis_summary", dvcMetadata.getAnalysisSummary());
metadata.put("source_code_repo", dvcMetadata.getRepoUrl());
// ... and so on
return metadata;
```
  The stream approach is fine if you have a variable number of entries or if you're doing more complex transformations, but for a fixed set of entries, the direct approach is often clearer.
- Issue: Using IllegalStateException for invalid data is acceptable, but consider creating custom exception types (e.g., InvalidDvcMetadataException) to provide more context and allow for more specific error handling. This improves the clarity of what went wrong.
- Issue: the validateIssuesCount uses Integer.parseInt and catches NumberFormatException.
  
  Suggestion: if issuesCount comes directly from user input, consider using a more robust input validation library or writing a function that does more than just check if it's a number. For example, is it safe to parse a very large integer that might cause an integer overflow downstream?
- Minor Issue: The logging statements are good, but ensure consistent use of logging levels (e.g., DEBUG, INFO, WARN, ERROR) based on the severity of the message.

These are my initial observations. Let me know if you have any specific questions or want me to elaborate on any of these points.

Okay, I've reviewed the code differences. Here's a summary of potential issues and suggestions:

Review Summary:

The primary changes seem to be the removal of code related to DVC (Data Version Control) metadata and its associated processing. Specifically, code has been removed from DvcMetadataService and JobService. This suggests a potential decoupling or shift in how DVC metadata is handled.

File-Specific Comments:

File: src/main/java/com/redhat/sast/api/service/DvcMetadataService.java

Issue: Entire file removed.
Suggestion:
- It's critical to understand why this file was removed. Was this functionality moved elsewhere? Was it deemed unnecessary?
- If the DVC metadata functionality is still required, ensure that the equivalent logic exists in another service/component and that all dependencies have been updated accordingly.
- If the DVC metadata functionality is no longer required, consider adding a comment to the commit message and/or PR description explaining the rationale for removal to avoid confusion in the future.

File: src/main/java/com/redhat/sast/api/service/JobService.java

Issue: updateJobDvcMetadata method removed.
Suggestion:
- Similar to the DvcMetadataService, determine where this functionality has been moved.
- If this logic is moved, make sure the call sites are updated accordingly.
- If no longer required, consider adding a comment in commit message.

File: src/main/java/com/redhat/sast/api/service/PlatformService.java

Issue: Dependency on DvcMetadataService and DataArtifactService removed, and the PipelineRunWatcher no longer uses the DvcMetadataService
Suggestion:
- Verify that the PipelineRunWatcher is still functioning correctly without the DVC metadata integration. Make sure all dependencies are cleaned up.

In general, for such a removal-heavy PR, ensure that there is a clear explanation of the changes, the rationale behind them, and confirmation that any dependent functionality has been properly migrated or is no longer needed. This will improve the maintainability and understandability of the codebase.

Okay, I've reviewed the changes. Here's a summary of the potential issues and suggestions:

Review Summary:

The changes primarily involve adding a PipelineRunWatcher, modifying database schema generation, and adding DVC metadata fields. The watcher seems reasonable, but the database changes raise a few questions and require closer scrutiny. Specifically, the change to drop-and-create is potentially dangerous for production environments.

File-Specific Comments:

src/main/java/com/redhat/ecosystemappeng/sast_ai_orchestrator/services/JobService.java
- Consider adding a comment explaining why the PipelineRunWatcher is necessary and what it's watching for. This will help future developers understand the purpose of this component.
- Add try-catch block or handle exception for pipelineRunWatcher.watch to properly handle exceptions.
src/main/resources/application.properties
- Critical Issue: Changing quarkus.hibernate-orm.database.generation to drop-and-create is extremely dangerous for any environment where data persistence is important (e.g., production, staging). This setting will wipe out the entire database on application startup.
- Recommendation: Revert this change immediately unless this is ONLY intended for local development/testing. Even for testing, consider using a dedicated testing database to avoid accidental data loss. If schema changes are needed, use Flyway migrations. The original settings (validate and migrate-at-start=true) were much safer.
- Explanation: validate ensures the database schema matches the entity definitions, and migrate-at-start=true applies any pending Flyway migrations. This approach is much more controlled and avoids accidental data loss.
src/main/resources/db/migration/V001__initial_schema.sql
- This file is being deleted. Is this intentional? If so, why? Deleting the initial schema might be problematic if you're relying on Flyway to manage your database schema. Consider keeping this and modifying it instead if the schema needs to evolve.
src/main/resources/db/migration/V002__add_dvc_metadata_fields.sql
- This file adds DVC metadata fields, which is good. Just ensure that the column types are appropriate for the data they will store (e.g., VARCHAR(255) might be too short for some URLs or metadata).
- Consider adding comments to the SQL file explaining the purpose of these new fields. This will improve maintainability.

In summary, the most critical issue is the database schema generation setting. Please address that immediately to prevent potential data loss. The other comments are suggestions for improved clarity and maintainability.

Okay, I have reviewed the SQL migration files being deleted. Here's a summary of potential issues and suggestions, assuming these migrations were intended to be applied:

Review Summary:

The deletion of these migration files (V002 and V003) is concerning. These files appear to be adding important DVC metadata tracking fields to the job table and creating a data_artifacts table. Deleting them will remove this functionality. It is unusual to delete database migrations. Consider whether this is the correct action. If the functionality is no longer needed, consider if deprecating the columns is more appropriate, to avoid data loss.

File-Specific Comments:

src/main/resources/db/migration/V002__add_dvc_metadata_fields.sql
- Issue: Deleting this file removes the creation of the data_artifacts table and the addition of DVC-related columns to the job table.
- Suggestion: Determine why this migration is being removed. If the functionality is still needed, the file should be kept. If the functionality is no longer needed, consider deprecating the tables/columns instead of outright deleting them, especially if there's existing data.
src/main/resources/db/migration/V003__add_data_artifacts_id_column.sql
- Issue: This migration adds an auto-incrementing id column to the data_artifacts table and makes it the primary key. Deleting this file will prevent this crucial table modification.
- Suggestion: Similar to V002, determine why this is being removed. The addition of an id column is a common practice for database tables and can be important for performance and ORM frameworks. If V002 is kept, V003 should likely also be kept to properly structure the data_artifacts table.

Before proceeding, it's crucial to understand why these migrations are being deleted. Are they being replaced by other migrations? Is the functionality obsolete? The answers to these questions will determine the appropriate course of action.

Okay, I understand. I'm ready to review the code differences you'll be providing, keeping in mind clarity, simplicity, efficiency, and security. I'll provide a summary and file-specific comments as needed. Let's proceed.

</details>

README.md

deploy/argocd/application.yaml

deploy/sast-ai-chart/values.yaml

Signed-off-by: Jude Niroshan <[email protected]>

github-actions

This Pull Request significantly improves deployment clarity and maintainability, particularly with the refactored `Makefile` and Helm chart `values` separation. However, it introduces **critical security and operational issues** that require immediate attention.

Pressing Issues & Code Suggestions:

CRITICAL: Hardcoded Database Passwords:
- Issue: Passwords are hardcoded in deploy/sast-ai-chart/values-dev.yaml and values-prod.yaml. This is a severe security vulnerability for both development and production.
- Suggestion: Immediately remove all hardcoded passwords. Implement a robust secrets management solution (e.g., Kubernetes Secrets, Helm Secrets plugin, Vault) to inject these values securely at deployment time.
Deployment Strategy & ArgoCD Changes (Unexplained):
- Issue: The deploy/argocd/application.yaml file has been deleted, and ArgoCD cleanup logic removed from Makefile, without explanation. This is a major change to the deployment strategy and will likely lead to orphaned ArgoCD resources if still used.
- Suggestion: Clearly explain the new deployment strategy for the sast-ai-orchestrator-syncer component. If ArgoCD is still in use, reintroduce the application resource or ensure it's managed by a higher-level GitOps pattern. Reintroduce ArgoCD cleanup logic if applicable.
latest Docker Tag Collision:
- Issue: build-dev-image.yml now tags development images as latest, potentially overwriting the latest stable release tag from build-release-image.yml. This creates non-deterministic and unstable deployments.
- Suggestion: Revert latest tagging for dev builds; use latest-dev or branch-specific tags. Define a consistent, global strategy for the latest tag (ideally, only for the latest stable release).
JobService.java Functional Change:
- Issue: The removal of job.setGSheetUrl(inputSourceUrl); for OSH_SCAN job types in src/main/java/com/redhat/sast/api/service/JobService.java is a significant functional change. The intent and impact on downstream consumers are unclear.
- Suggestion: Clarify why GSheetUrl is no longer set for OSH_SCAN. Document whether GSheetUrl is now irrelevant for these jobs, or if inputSourceUrl serves a new, generic purpose. Confirm no regressions for downstream systems expecting this field. Add a concise code comment for future clarity.
Makefile Clarity & Maintainability:
- Issue: DEV_NAMESPACE and PROD_NAMESPACE are used but not defined, and the .DEFAULT_GOAL := help is set, but the help target itself is missing.
- Suggestion: Define DEV_NAMESPACE ?= sast-ai-dev and PROD_NAMESPACE ?= sast-ai-prod at the top of the Makefile. Implement the help target to list available commands.

Overall Positive Aspects:

The PR significantly improves README.md documentation, centralizes environment configurations in Makefile variables, and refactors Helm chart values for better organization and clarity. Environment-specific deploy and robust clean targets enhance operational safety and reduce duplication.

Review Summary:

The changes primarily focus on updating build and release workflows, particularly around Docker image tagging and Helm chart value management. A significant issue found is a potential collision and confusion regarding the latest Docker image tag, which could lead to non-deterministic deployments. The move to values-prod.yaml for Helm charts is a positive step towards clearer environment separation.

`.github/workflows/build-dev-image.yml`

Issue: latest tag collision. The change updates the dev image to be tagged as latest on the default branch. This is problematic if the release workflow (build-release-image.yml) also tags images as latest. The latest tag should ideally point to the latest stable release image. Allowing a development build to overwrite latest can lead to an unstable or incorrect image being pulled for production environments, which is a significant operational and potential security risk.
Suggestion: Revert the latest tag for dev builds. Use latest-dev or branch-specific tags for development images. If the default branch is the release branch, then this workflow should be renamed and configured as a release workflow, or the current release workflow should handle the default branch's latest tag.

`.github/workflows/build-release-image.yml`

Positive Change: Moving from values.yaml to values-prod.yaml for setting the release tag. This is a good improvement as it explicitly separates production-specific configurations, enhancing clarity and maintainability. All corresponding changes for sed, grep, git add, and commit messages are consistent with this.
Issue: Inconsistent latest tag communication (related to build-dev-image.yml issue). The release notes now remove the mention of the latest tag and emphasize pulling specific versions for production. While good practice to recommend specific versions, this change, combined with build-dev-image.yml now potentially tagging latest, creates an inconsistent and confusing tagging strategy.
Suggestion:
1. Clarify the purpose of values.yaml if it still exists. If it's no longer used for production, consider removing it or clearly defining its role (e.g., default values, local development).
2. Once the latest tag collision issue (mentioned in build-dev-image.yml review) is resolved, ensure the release notes accurately reflect which tags are pushed by the release pipeline. If the release pipeline does push a latest tag (which is common and often desired for the latest stable release), it should be explicitly mentioned in the release notes. If it doesn't, that's fine, but the tagging strategy needs to be globally consistent.
  Review Summary:
  This pull request significantly enhances the project's documentation and deployment process, making it much clearer and easier to manage. The README.md has been overhauled to provide explicit environment strategies, streamlined deployment steps, and direct access instructions. The deploy/Makefile changes support this by centralizing environment-specific configurations. These are excellent improvements for developer experience and operational clarity. No major security concerns, inefficient algorithms, or problematic exception handling were identified.

File: GitHub Actions Workflow (inline snippet generating release notes)

Improvement: The clarification that latest is for development use, coupled with the new ### 🚀 Deployment section in the release notes, greatly improves the clarity for users. It clearly differentiates between production (tagged releases) and development (latest) deployments.
Suggestion: No specific suggestions. This change improves the release notes content.

File: README.md

Improvement: The additions of ### Environment Strategy, ### Quick Deployment, ### Container Images, and ### Environment-Specific Access sections are a significant positive change. They provide a clear, structured, and comprehensive guide for understanding and interacting with the different environments. Replacing the deploy/README.md reference with make help is also a great usability improvement for discovering commands.
Suggestion: No specific suggestions. This is a very well-executed documentation update.

File: deploy/Makefile

Improvement: Introducing DEV_NAMESPACE and PROD_NAMESPACE variables centralizes environment names, making the Makefile more maintainable and less prone to errors. Updating the CURRENT_NAMESPACE fallback to $(DEV_NAMESPACE) is consistent with the new environment strategy.
Suggestion: No specific suggestions. This is a good practice for maintainability.
Here's a review of the provided Makefile diff:

Review Summary

The changes in this Makefile are a significant improvement in terms of managing deployments across different environments (development and production). The introduction of dedicated targets like deploy-dev, deploy-prod, upgrade-dev, and upgrade-prod, along with an internal, parameterized _deploy and _upgrade target, greatly reduces duplication and enhances clarity. Using environment-specific values files is a strong step towards secure and maintainable configuration management.

Issues and Suggestions

File: Makefile

Clarity & Maintainability: Undefined Namespace Variables
- Issue: The variables DEV_NAMESPACE and PROD_NAMESPACE are used but not defined within this Makefile snippet. While they might be environment variables, it's best practice to define them with default values explicitly in the Makefile itself for clarity and easier understanding, especially for new contributors.
- Suggestion: Add these variable definitions at the top of the Makefile.
```
DEV_NAMESPACE ?= sast-ai-dev
PROD_NAMESPACE ?= sast-ai-prod
```
Security & Configuration: Namespace Variable Sourcing
- Issue: While moving from hardcoded namespace to variables like $(DEV_NAMESPACE) is good, the origin and immutability of these variables are critical for security in a production context. If CURRENT_NAMESPACE or DEV_NAMESPACE/PROD_NAMESPACE can be easily overridden by an attacker or misconfigured by accident, it could lead to deployments in unintended namespaces.
- Suggestion: Ensure that DEV_NAMESPACE and PROD_NAMESPACE are set from a trusted source (e.g., CI/CD environment variables that are strictly controlled) and are not easily mutable, especially for production deployments.
Clarity: Missing help Target Definition
- Issue: The .DEFAULT_GOAL := help is set, indicating an intention to have a help target, but the help target itself is not provided in the diff.
- Suggestion: Implement the help target to list all available commands with their ## comments. This significantly improves the discoverability and usability of the Makefile.
Minor: Error Suppression (2>/dev/null)
- Issue: Suppressing stderr for helm repo add and helm repo update (and other commands not fully shown) can hide important diagnostic information, especially during initial setup or debugging.
- Suggestion: While common for clean CI output, consider if this level of suppression is always desired. For development builds, sometimes seeing all output can be beneficial. This is a minor point and often a deliberate choice.
Improvement: Passing NAMESPACE to sub-targets
- Observation: The change to explicitly pass NAMESPACE=$(NAMESPACE) to wait-pods, argocd, and show-access is an excellent improvement. It removes ambiguity and ensures these subsequent actions operate on the correct, environment-specific namespace. This improves reliability and prevents cross-environment issues.
  Here's a review of the provided diff:

Review Summary

The changes introduce significant improvements in clarity, organization, and robustness for the cleanup process. Splitting the clean target into environment-specific targets (clean-dev, clean-prod) and introducing a common internal _clean target is an excellent refactoring that reduces duplication and enhances maintainability. The cleanup commands are made more resilient with error handling.

The primary concern is the complete removal of the ArgoCD application cleanup logic. If ArgoCD is still part of the deployment ecosystem, this will lead to orphaned ArgoCD application resources.

File: `Makefile`

Issue: Potential gap in ArgoCD cleanup.

Comment: The entire block responsible for checking and deleting ArgoCD Application resources (applications.argoproj.io) has been removed. If ArgoCD is still used for deployments, removing these application resources is crucial for a complete cleanup, preventing orphaned resources, and ensuring smooth re-deployments.
Suggestion: Reintroduce the ArgoCD application cleanup logic into a dedicated target (e.g., _clean_argocd) or integrate it back into _clean if applicable to all environments, ensuring it's conditionally executed if ArgoCD CRDs exist.

Improvement: Enhanced Clarity and Reusability

Comment: The introduction of clean-dev and clean-prod calling a common _clean target is a great improvement. It reduces duplication and makes the Makefile more explicit about environment-specific operations.
Suggestion: N/A (Excellent change)

Improvement: Robust Cleanup Commands

Comment: The _clean target's commands for deleting resources by label and then performing a "force clean" loop are very thorough and robust. The use of --ignore-not-found=true 2>/dev/null || true makes the cleanup resilient to missing resources, which is excellent for cleanup tasks. xargs -r is also a good safety measure.
Suggestion: N/A (Excellent change)

Improvement: Clearer Output

Comment: Adding emojis to status messages (✅ Upgrade completed!, 🧹 Removing..., 📊 SAST AI Deployment) and the specific "Deployment removed from $(NAMESPACE)" improves visual feedback and clarity.
Suggestion: N/A (Good small improvement)

Minor Note: Removed install alias

Comment: The install alias for deploy has been removed. If other scripts or documentation refer to make install, they will need to be updated.
Suggestion: Ensure all references to make install are updated to make deploy or the new appropriate target if deploy itself was also refactored.
Here's a review of the provided diffs:

Review Summary

The changes in this pull request generally improve the clarity and maintainability of the deployment tooling. The Makefile has been significantly enhanced with environment-specific commands, a useful status command, and a self-updating help message, which are all positive steps towards better operational practices. The addition of values-dev.yaml clearly separates development configurations, which is good.

The major point of concern is the deletion of deploy/argocd/application.yaml. While the other changes are improvements, removing a critical deployment configuration file like an ArgoCD Application without an explanation raises questions about the new deployment strategy and its implications for consistency, automation, and security.

Issue Comments per File

`Makefile`

Clarity & Simplicity:

Comment: The new environment-specific targets (deploy-dev, deploy-prod, etc.) and the status command significantly improve clarity and reduce ambiguity compared to generic deploy or upgrade commands. This is excellent for preventing errors, especially in critical environments.
Suggestion: None. This is a very good improvement.

Duplication & Complexity:

Comment: The awk based help target is a standard and effective way to keep the help message synchronized with the actual targets, reducing manual effort and potential for outdated documentation.
Suggestion: None.

Operational Security:

Comment: Providing separate, explicit commands for dev and prod environments (e.g., deploy-dev vs deploy-prod) inherently reduces the risk of accidental deployments or changes to the wrong environment. The status command aids in operational visibility, which is also a security best practice.
Suggestion: None.

`deploy/argocd/application.yaml` (Deleted)

Clarity & Deployment Strategy:

Comment: The deletion of this ArgoCD Application resource is a significant change to how sast-ai-orchestrator-syncer is deployed and managed via GitOps. This change isn't explained within the diff, making it unclear what the new deployment strategy is.
Suggestion: Please provide a clear explanation for this deletion.
- Is ArgoCD no longer used for this component, and if so, what is the new deployment method (e.g., manual Helm, different CI/CD pipeline)?
- If ArgoCD is still used, is this application now managed by a higher-level "App of Apps" pattern, or a different ArgoCD instance/project?
- Understanding the new strategy is crucial for reviewing its implications for automation, consistency, and security.

Security Implications:

Comment: Moving away from a GitOps-managed deployment (like ArgoCD syncing from a repo) can potentially introduce new security considerations. GitOps provides a strong audit trail and desired state enforcement. If the new deployment method is less automated or less transparent, it could reduce security posture.
Suggestion: Ensure the replacement deployment strategy maintains or improves the current level of automation, traceability, and security controls. If it's manual, consider the increased risk of human error and lack of auditability.

`deploy/sast-ai-chart/values-dev.yaml` (New File)

Clarity & Best Practices:

Comment: This file correctly separates development-specific overrides (like pullPolicy: Always and tag: "latest"). This is a standard and excellent practice for managing environment-specific configurations.
Suggestion: None. This is well-implemented.

Efficiency:

Comment: Using pullPolicy: Always in development ensures developers always get the latest image, which is efficient for rapid development cycles, avoiding stale image issues.
Suggestion: None.
Review Summary:

The PR introduces environment-specific Helm values files for dev and prod. The structure of using separate files for overrides is a good practice for managing environment configurations. Resource allocations and feature enablement (HPA, PDB) are appropriately differentiated between dev and prod, showing good consideration for environment specific needs.

However, there is a critical security vulnerability identified in both files: hardcoded database passwords. This is a major issue that needs to be addressed immediately.

`deploy/sast-ai-chart/values-dev.yaml`

Issue Comments:

Security Vulnerability: Hardcoded Passwords
- Problem: The postgresql.auth.postgresPassword and postgresql.auth.password values are hardcoded directly in the values-dev.yaml file (postgres-dev, quarkus-dev). Even for a development environment, committing passwords to version control is a significant security risk. These are likely weak/default passwords.
- Suggestion: These passwords should never be committed to Git. Utilize Kubernetes Secrets, a secrets management system (e.g., Vault, Azure Key Vault, AWS Secrets Manager), or the Helm Secrets plugin to inject these values securely at deployment time.
Minor: No Newline at End of File
- Problem: The file is missing a newline at the end, which can sometimes cause minor issues with tools or lead to inconsistent formatting.
- Suggestion: Add a newline at the end of the file.

`deploy/sast-ai-chart/values-prod.yaml`

Issue Comments:

Critical Security Vulnerability: Hardcoded Passwords
- Problem: Similar to the dev environment, postgresql.auth.postgresPassword and postgresql.auth.password are hardcoded in values-prod.yaml (postgres-prod-secure, quarkus-prod-secure). Even with "secure" in the name, this is a severe security flaw for a production environment. Committed passwords are a direct path to compromise if the repository is ever breached or misconfigured.
- Suggestion: This is a critical fix required before merging. These passwords must not be committed to Git. Implement a robust secrets management strategy using Kubernetes Secrets, a dedicated secrets management system (e.g., Vault, Azure Key Vault, AWS Secrets Manager), or the Helm Secrets plugin to manage and inject sensitive credentials securely at deployment time.
Clarity: Image Tag Management
- Problem: The comment states tag: "v1.0.1" # Production uses release tags (updated by CI/CD). While clear, ensure the CI/CD pipeline truly enforces this and doesn't allow manual overrides without proper review, especially for production.
- Suggestion: This is more of a process note, but ensure the CI/CD pipeline rigidly updates this tag upon release and that manual updates to this file are strictly controlled to prevent accidental deployments of incorrect versions.

Review Summary

This pull request introduces significant improvements to the Helm chart's organization and security posture by refactoring values.yaml. The primary goal of separating shared configuration from environment-specific overrides is achieved effectively, leading to enhanced clarity, maintainability, and security.

Key Strengths:

Improved Organization: Clearly separates shared settings from environment-specific ones, making the chart easier to manage across different deployments (dev, prod, etc.).
Enhanced Clarity: New comments explicitly state the purpose of the values.yaml file and which sections are shared or intended for override.
Major Security Improvement: The removal of hardcoded PostgreSQL credentials from the shared values.yaml is a critical security best practice.

Suggestions for Further Improvement:

Security Validation: While the removal of credentials is good, ensure the new mechanism for managing them (in environment-specific files) leverages Kubernetes Secrets or another secure secrets management solution.
Specific Configuration Review: Re-evaluate certain security-related flags that remain in the shared values.yaml to confirm they are universally applicable and secure.

File Comments

`deploy/sast-ai-chart/values.yaml`

Clarity & Maintainability (Positive):
The refactoring to clearly delineate shared configurations from those overridden in environment-specific files (values-dev.yaml, values-prod.yaml) is excellent. This significantly improves the chart's clarity and maintainability. The updated comments throughout the file further enhance understanding.
Security (Major Improvement):
Removing sensitive defaults like postgresql.auth credentials from the shared values.yaml is a substantial security enhancement. This prevents hardcoding credentials in a potentially widely distributed base configuration. Please ensure these credentials are now managed securely in the environment-specific values files, ideally through Kubernetes Secrets or a secrets management system, rather than directly in plaintext in values-dev.yaml or values-prod.yaml.
Security (Suggestion):
Consider moving QUARKUS_KUBERNETES_CLIENT_TRUST_CERTS: false to environment-specific values files if its setting might vary or has specific security implications for certain environments. Generally, trusting certificates is a good security practice to prevent man-in-the-middle attacks, so if this is false, it should be a deliberate, documented, and environment-specific decision.
Style (Minor Suggestion):
Add a newline at the end of the file. This is a common best practice (\ No newline at end of file indicates its absence).
Review Summary:

The changes primarily focus on improving clarity in the Helm chart's values.yaml (the tor file) by adding context to various configuration sections. There's also a significant functional change in JobService.java related to how OSH_SCAN jobs are handled.

Overall Impression:
The values.yaml changes are positive for maintainability and understanding. The change in JobService requires more context to ensure functional correctness and no unintended side effects.

File: `tor`

Comments:

Clarity: The additions of (shared) and (disabled by default) to various configuration sections (RBAC, ConfigMap, HPA, PDB, Monitoring) are excellent. They significantly improve the clarity and understanding of these Helm values for anyone using or maintaining the chart.
Reordering: The networkPolicy block has been moved from above monitoring to below it. While this is a functionally inert change, it's good practice to have a consistent logical ordering. If there's a new logical grouping, it's fine. If not, consider keeping the original order or establishing a clear standard.
Whitespace: Adding the missing newline at the end of the file is a good standard practice and prevents potential issues with some tools.

Suggestions:

Logical Grouping (Minor): If the reordering of networkPolicy was intentional for a new logical grouping, perhaps add a small comment explaining the new structure or ensure similar configuration blocks are consistently grouped throughout the file. If it was accidental, it's fine as is, but consistency helps.

File: `src/main/java/com/redhat/sast/api/service/JobService.java`

Comments:

Functional Change: The removal of job.setGSheetUrl(inputSourceUrl); for OSH_SCAN input types is a significant functional change. Previously, the inputSourceUrl was explicitly stored as GSheetUrl for OSH scans. Now, it will likely be null or an empty string for GSheetUrl if no other logic populates it, while inputSourceUrl itself is used elsewhere in the debug log.
Potential Impact: This change could lead to regressions if any downstream components (other services, UI, reports) expect the GSheetUrl field to be populated for OSH_SCAN job types.

Suggestions:

Clarify Intent and Impact:
- Why was job.setGSheetUrl(inputSourceUrl); removed for OSH_SCAN? Is GSheetUrl no longer relevant for OSH scans, or is inputSourceUrl now serving a different purpose (e.g., a generic OSH report URL) that is not a Google Sheet?
- Is the GSheetUrl field on the Job object still necessary? If it's only used for actual Google Sheet input types and not OSH scans, then this change makes sense. If inputSourceUrl is a more generic "source URL", then GSheetUrl might be a misnomer anyway.
- Are there any downstream consumers of GSheetUrl for OSH_SCAN jobs that need to be updated or considered? This is crucial to prevent unexpected bugs.
- Consider Renaming (if applicable): If inputSourceUrl is truly generic and GSheetUrl is now only for Google Sheets, consider if the GSheetUrl field itself needs to be renamed on the Job object to something more specific like googleSheetUrl or if inputSourceUrl should be saved to a more generic field like reportUrl for OSH scans.
Add Comment (If Context isn't obvious): If the reasoning isn't immediately clear from the surrounding code or existing documentation, a concise code comment explaining why GSheetUrl is no longer set for OSH_SCAN jobs would be beneficial.

Signed-off-by: Jude Niroshan <[email protected]>

github-actions

Here's a summary of the review, focusing on pressing issues and code suggestions:

Review Summary:

This PR introduces significant improvements in documentation, Makefile organization, and environment-specific Helm chart configurations. However, it introduces critical security vulnerabilities and operational inconsistencies related to Docker image tagging and hardcoded database credentials. The removal of ArgoCD application definitions also needs urgent clarification.

Pressing Issues & Code Suggestions:

Critical Security Risk: Inconsistent latest Docker Image Tagging
- Issue: The latest tag is now assigned to development builds (build-dev-image.yml), while the release workflow (build-release-image.yml) explicitly removes latest tagging. This means latest will point to unstable dev builds, creating a high security risk for consumers expecting a stable release.
- Suggestion:
  - build-dev-image.yml: Revert type=raw,value=latest,enable={{is_default_branch}}. Use latest-dev or main-latest for development builds.
  - build-release-image.yml: Explicitly add type=raw,value=latest to the docker/metadata-action for release builds. This must override any development latest tags.
Critical Security Vulnerability: Hardcoded Production Database Passwords
- Issue: postgresql.auth.postgresPassword and postgresql.auth.password are hardcoded directly in values-prod.yaml. This is a severe security flaw, exposing production credentials in source control.
- Suggestion:
  - deploy/sast-ai-chart/values-prod.yaml: Immediately remove all hardcoded production passwords. Implement a robust secrets management solution (e.g., Kubernetes Secrets, Vault, cloud secret managers) and update the Helm chart to reference these secrets instead.
  - deploy/sast-ai-chart/values-dev.yaml: While less critical, avoid hardcoding passwords even for development. Use secrets or a secure default generation mechanism.
Major Architectural Change: ArgoCD Application Deletion
- Issue: The deploy/argocd/application.yaml file has been deleted without clear context, indicating a significant change in how sast-ai-orchestrator-syncer is deployed. This could lead to orphaned deployments or a lack of managed deployments.
- Suggestion: Clarify the new deployment mechanism for this component. Confirm if ArgoCD is still used for this, or if a different CI/CD pipeline or manual deployment is now in place. If ArgoCD is still relevant, similar cleanup logic might be needed in the Makefile.

Key Improvements (Positive Changes):

Makefile Refactoring: Excellent work introducing internal targets (_deploy, _upgrade, _clean) to reduce duplication and improve modularity. The help and status targets greatly enhance user experience.
Environment-Specific Helm Values: The introduction and use of values-dev.yaml and values-prod.yaml is a major positive, clearly separating configurations for different environments.
Documentation Clarity: Updates to README.md and configuration comments significantly improve clarity and guidance for developers.
OSH Scan Completeness: Fetching scan-results-all.js improves the completeness of security analysis data.

Review Summary:

The changes primarily affect image tagging strategies and Helm chart updates within CI/CD workflows. While the shift to values-prod.yaml in the release pipeline is a positive step for separating production configurations, the changes to how the latest Docker image tag is handled are problematic and introduce potential confusion and security risks.

File: .github/workflows/build-dev-image.yml

Issue: latest tag assignment for development builds
- Comment: Changing the development image tagging to type=raw,value=latest,enable={{is_default_branch}} means the latest tag will now point to the most recent development build whenever the default branch is updated. This is a common anti-pattern. The latest tag is conventionally reserved for the most recent stable release of an application. Assigning it to a development build can lead to consumers (or automated systems) pulling an unstable, unvetted, or potentially vulnerable image when they expect a stable release.
- Suggestion: Revert this change to use latest-dev or another specific tag (e.g., main-latest, snapshot) for development builds on the default branch. The latest tag should be managed exclusively by the release workflow to point to the latest stable release.

File: .github/workflows/build-release-image.yml

Issue: Inconsistent latest tag strategy (Security Concern)
- Comment: The removal of latest from the release notes and the implicit lack of latest tagging in the release workflow (when combined with the build-dev-image.yml change) means the latest tag will never point to a stable release. This creates a security risk where anyone pulling latest will get a development build, not a vetted production-ready release.
- Suggestion: The build-release-image.yml workflow must explicitly tag the release image with latest. This ensures that latest consistently points to the most recent stable version, overriding any development latest tags. Update the docker/metadata-action for release builds to include type=raw,value=latest. Also, the release notes should then reflect that latest is available and points to the current release.
Improvement: Use of values-prod.yaml
- Comment: Shifting the Helm chart tag update to deploy/sast-ai-chart/values-prod.yaml instead of the generic values.yaml is a good practice. It explicitly separates production-specific configurations, reducing the risk of deploying non-production settings to a production environment. This enhances clarity and security for production deployments.
- Suggestion: No changes needed here, this is a positive improvement.
Improvement: Commit Message Clarity
- Comment: The updated commit message Update production versions to ${{ github.event.release.tag_name }} is clearer, indicating that the changes specifically target production configurations.
- Suggestion: No changes needed.

Review Summary

This Pull Request introduces significant improvements to the project's deployment documentation and automation. The changes clearly define environment strategies, simplify deployment commands, and enhance the overall clarity for developers and operations teams. This is a strong positive change.

File by File Review

GitHub Actions / Release body content (implied context)

Clarity & Simplicity:
- Adding make deploy-prod and make deploy-dev instructions directly to the release notes makes deployment for specific releases very clear and straightforward. This is excellent for users consuming the release.
- Changing the comment from # Pull latest to # For development, use the latest tag provides better context for the action being performed.
Suggestion: No issues found. This is a good improvement.

`README.md`

Clarity & Simplicity:
- The new ### Environment Strategy section is exceptionally clear. It explicitly defines the purpose, image usage, update triggers, and characteristics (logging, resource limits) for both Development and Production environments. This is crucial for maintainability and avoiding confusion.
- The ### Quick Deployment section, using make deploy-dev and make deploy-prod, simplifies the user experience significantly.
- Separating ### Container Images into development and production clearly shows which tags to use.
- Updating ### Docker Deployment to provide both latest (dev) and versioned (prod) examples is very helpful.
- The ### Environment-Specific Access section provides practical commands for accessing deployed applications, which is a great addition for post-deployment verification.
Security:
- The clear distinction between dev and prod environments, including different image tags (latest vs. vX.Y.Z) and update mechanisms (main branch push vs. GitHub releases), is an excellent security and operational practice. It reduces the risk of accidental deployment of untested code to production and ensures a more controlled release process for production artifacts.
Suggestion: No issues found. These changes greatly enhance the project's documentation quality.

`deploy/Makefile`

Clarity & Simplicity:
- Introducing DEV_NAMESPACE and PROD_NAMESPACE variables makes the Makefile more maintainable and readable. It centralizes environment-specific configurations.
- This change is foundational for the simplified make deploy-dev and make deploy-prod commands highlighted in the README.md and release notes, contributing to a much simpler user experience.
Suggestion:
- (Minor, assuming the rest of the Makefile) Ensure that the deploy-dev and deploy-prod targets consistently use these new namespace variables (e.g., via -n $(DEV_NAMESPACE) or oc project $(DEV_NAMESPACE)) to enforce the environment separation and prevent namespace mismatches. The provided snippet only shows the variable definition.
  Review Summary:
  This pull request significantly improves the Makefile by refactoring deployment and upgrade processes for different environments (development and production). It introduces internal targets (_deploy, _upgrade) to reduce code duplication, uses environment-specific Helm values files, and enhances clarity with more descriptive messages. The changes lead to a more maintainable, robust, and explicit deployment system.

Issue Comments:

`Makefile`

Clarity & Duplication: Excellent refactoring by introducing _deploy and _upgrade targets, which significantly reduces duplication between deploy-dev/deploy-prod and upgrade-dev/upgrade-prod. This makes the Makefile much more modular and easier to maintain.
Environment-Specific Configuration: The use of VALUES_FILE (e.g., values-dev.yaml, values-prod.yaml) for Helm installs and upgrades is a major improvement. This allows for clear separation of configurations between environments, which is crucial for security and operational consistency.
Parameterization: Passing NAMESPACE, RELEASE_NAME, and VALUES_FILE as arguments to the internal targets (_deploy, _upgrade) and other helper targets (wait-pods, argocd, show-access) is a great practice, ensuring that commands operate on the correct context.
Suggestion (Clarity): The variables DEV_NAMESPACE and PROD_NAMESPACE are used, but their definition is not present in this diff. For better clarity and self-containment, it's recommended to either define them with default values in the Makefile (e.g., DEV_NAMESPACE ?= sast-ai-dev) or add a comment clarifying that they are expected to be set as environment variables.
Suggestion (Consistency): The success/completion messages are somewhat split between the public deploy-dev/deploy-prod targets (which print initial messages) and the internal _deploy target (which prints the completion message). Consider consolidating the messaging flow (e.g., let _deploy print all "Deploying..." and "Deployment completed!" messages) for a more consistent user experience.
Review Summary:

The changes primarily refactor the Makefile's cleanup logic, introducing dedicated targets for development and production environments, and centralizing the actual cleanup steps into an internal _clean target. There are also minor improvements to command output and the upgrade target.

Overall, the refactoring for cleanup is a positive change, promoting clarity and reducing duplication. However, there's a significant removal of ArgoCD application cleanup logic that needs clarification.

File: Makefile

Clarity & Simplicity:
- Comment: The introduction of clean-dev, clean-prod, and _clean targets significantly improves the organization and clarity of the cleanup process. It clearly separates concerns for different environments and centralizes the common cleanup steps.
- Suggestion: N/A, this is a good improvement.
Duplication & Complexity:
- Comment: The new _clean target effectively reduces duplication by consolidating the common steps for Helm uninstallation and Kubernetes resource cleanup. clean-dev and clean-prod now simply call _clean with environment-specific parameters.
- Suggestion: N/A, this is well-executed.
Robustness / Error Handling:
- Comment: The removal of the ArgoCD application cleanup logic from the clean process (if $(KUBECTL_CMD) get crd applications.argoproj.io...) is a significant change. If ArgoCD is still used in the deployment pipeline, this could lead to orphaned ArgoCD Applications after a make clean operation.
- Suggestion: Please clarify if ArgoCD is no longer part of the deployment/cleanup strategy, or if this cleanup is now handled elsewhere. If ArgoCD is still used, consider re-introducing similar logic to _clean or a separate clean-argocd target that clean-dev and clean-prod could also invoke.
- Comment: The updated upgrade target now calls wait-pods NAMESPACE=$(NAMESPACE) instead of a generic wait. This is more specific and likely more robust for ensuring pods are ready.
- Suggestion: N/A, good improvement.
Minor Improvements:
- Comment: Adding emojis (✅, 🧹) to output messages makes the console output more visually engaging and clear.
- Suggestion: N/A.
Potential Breaking Change:
- Comment: The install: deploy alias was removed. If any existing scripts or documentation refer to make install, this change will break them.
- Suggestion: Confirm if install was a widely used alias and if its removal is intentional, or if deploy is now the only entry point. If it's a breaking change, consider mentioning it in a release note or updating relevant documentation.
  Here's my review of the changes:

Review Summary:
The changes significantly improve the development experience by enhancing the Makefile for better command discoverability and status checks. The introduction of values-dev.yaml promotes better environment-specific configuration management. The deletion of the ArgoCD application file is a substantial change that requires further context regarding the new deployment strategy.

File: Makefile

Clarity & Simplicity: The added status target is clear and provides useful immediate feedback. The overhaul of the help command is excellent; it greatly improves clarity and discoverability of commands, making the Makefile much more user-friendly.
Suggestion: No immediate suggestions for improvement; these are good changes.

File: deploy/argocd/application.yaml (deleted)

Impact: Deleting the ArgoCD application definition indicates a significant change in the deployment strategy for sast-ai-orchestrator-syncer.
Security/Architecture: Please clarify the new deployment mechanism for this component. Is it now managed by a different ArgoCD application, a different CI/CD pipeline, or is it deployed manually? Understanding the new approach is crucial for reviewing its security posture and operational aspects.

File: deploy/sast-ai-chart/values-dev.yaml

Clarity: The file clearly defines development-specific overrides, which is good practice.
Fix: The tag: "l" value appears incomplete. It should likely be "latest" or a specific development build tag. Please complete this value.
Security (Minor): While pullPolicy: Always is common for development, using a floating tag like "latest" (if that's the intent) can sometimes lead to non-reproducible builds. For better consistency, even in dev, consider pinning to more specific, immutable image tags when feasible, even if pullPolicy: Always is retained to ensure the latest specific image is used.

Review Summary

The pull request introduces dedicated Helm values-dev.yaml and values-prod.yaml files, which is an excellent practice for managing environment-specific configurations. The differentiation in resource allocation, replica counts, HPA, PDB, and application settings between development and production is appropriate and well-defined.

However, there is a critical security vulnerability identified in how database credentials are managed. Hardcoding passwords directly in these configuration files, especially for production, is a severe security risk and goes against best practices.

Issue Comments

`deploy/sast-ai-chart/values-dev.yaml`

Issue: Hardcoded Passwords
- Comment: The postgresql.auth.postgresPassword and postgresql.auth.password values are hardcoded as "postgres-dev" and "quarkus-dev" respectively. While this is a development environment, hardcoding even simple credentials in source control is generally a bad practice. It can lead to bad habits, potential exposure, or accidental use in other environments.
- Suggestion: For all environments, including development, sensitive information like passwords should be managed using a secrets management solution (e.g., Kubernetes Secrets, OpenShift Secrets, HashiCorp Vault, cloud-specific secret managers). The Helm chart should then reference these secrets. If a simple default for dev is absolutely necessary for local testing, consider a more robust mechanism than a plain string in the values.yaml file, or ensure they are randomized/generated during deployment.
Issue: Missing Newline at End of File
- Comment: The file is missing a newline character at the end (\ No newline at end of file). This is a minor formatting issue.
- Suggestion: Add a newline at the end of the file for consistency with common file formatting standards.

`deploy/sast-ai-chart/values-prod.yaml`

Issue: Critical Security Vulnerability - Hardcoded Production Passwords
- Comment: The postgresql.auth.postgresPassword ("postgres-prod-secure") and postgresql.auth.password ("quarkus-prod-secure") values are hardcoded directly in this production configuration file. This is a critical security vulnerability. Production credentials should never be stored in plaintext in source control. Anyone with access to the repository can see these passwords, making the database highly susceptible to unauthorized access.
- Suggestion: Immediately address this. Production database credentials must be managed using a robust secrets management system (e.g., Kubernetes Secrets, OpenShift Secrets, HashiCorp Vault, AWS Secrets Manager, GCP Secret Manager). The Helm chart should be updated to reference these external secrets rather than embedding them directly in values-prod.yaml. This ensures that sensitive data is encrypted at rest and in transit, and access is tightly controlled.
Issue: Truncated serviceMonitor Configuration
- Comment: The monitoring.serviceMonitor configuration appears to be incomplete or truncated at the end of the file (serviceMon).
- Suggestion: Please complete the serviceMonitor configuration to ensure it's valid and correctly defined.
  Review Summary:

This Pull Request introduces significant improvements to the Helm chart's configuration management and monitoring capabilities. The most notable change is the refactoring of values.yaml to focus on shared configurations, delegating environment-specific settings to dedicated files (e.g., values-dev.yaml, values-prod.yaml). This is a highly recommended practice that enhances clarity, reduces complexity, and improves the secure handling of configurations across different environments.

A critical security improvement is the removal of hardcoded default PostgreSQL credentials from values.yaml, which forces more secure handling of sensitive database access information. The addition of a configurable Prometheus ServiceMonitor is also a valuable enhancement for operational visibility.

Overall, this PR is a strong positive change, greatly improving the maintainability, security, and flexibility of the application's deployment.

Issue Comments:

File: deploy/sast-ai-chart/templates/prometheus-servicemonitor.yaml

No major issues found. This file is well-implemented.
The changes correctly conditionalize the ServiceMonitor resource and make its scraping interval and timeout configurable, which is good practice.

File: deploy/sast-ai-chart/values.yaml

Issue: Missing Newline at End of File
- Comment: It's a common best practice in YAML files to ensure they end with a newline character. While not a functional bug, it helps with file manipulation and version control consistency.
- Suggestion: Add a newline at the very end of the values.yaml file.
Security Improvement (Not an Issue): Removal of Hardcoded DB Credentials
- Comment: The removal of default postgresPassword, username, password, and database from the postgresql.auth section is an excellent security enhancement. It prevents sensitive information from being hardcoded in a shared configuration file and encourages the use of more secure methods (e.g., Kubernetes Secrets) for managing database credentials, likely in environment-specific values files or directly referenced secrets.
- Suggestion: Ensure that the documentation or the environment-specific values files clearly outline how these sensitive database credentials should now be provided and secured.
  Here's a review of the provided diffs:

Review Summary

The changes primarily focus on improving clarity in configuration files, a minor refactoring related to GSheetUrl in JobService, and an important update to fetch more comprehensive scan results (scan-results-all.js) in OshJobCreationService. The configuration changes are positive for documentation, and the OSH URL update generally improves security data completeness. The GSheetUrl removal might need a bit more context for full clarity.

Issue Comments

File: configuration

Comment: Good improvement in clarity for configuration parameters by adding (shared) and (disabled by default). The reordering of Network Policy and adding a newline at EOF are good practices. No functional changes, only improved documentation.

File: src/main/java/com/redhat/sast/api/service/JobService.java

Comment: The removal of job.setGSheetUrl(inputSourceUrl); for OSH_SCAN type jobs is noted.
Suggestion: Ensure that inputSourceUrl is not intended to be a GSheet URL when InputSourceType.OSH_SCAN is set, or clarify what inputSourceUrl now represents in this context. If GSheetUrl is solely for Google Sheet inputs, this is a clean separation. If inputSourceUrl could still be a GSheet for OSH, its removal might discard relevant metadata.

File: src/main/java/com/redhat/sast/api/service/osh/OshJobCreationService.java

Comment: Changing the OSH report URL from scan-results-imp.js to scan-results-all.js is a positive step from a security analysis perspective as it likely ensures that all available scan results are retrieved, rather than just "important" ones. This improves data completeness for SAST analysis.
Suggestion: Verify the performance impact and expected size difference between scan-results-imp.js and scan-results-all.js. Also, ensure the OSH backend system is guaranteed to provide scan-results-all.js at this endpoint consistently.
src/main/resources/application.properties

No issues. This is a simple configuration update for the default primary JSON file name, which is clear and straightforward.

Signed-off-by: Jude Niroshan <[email protected]>

github-actions

This PR significantly improves clarity, maintainability, and environment separation, especially for development and production deployments. The removal of hardcoded PostgreSQL credentials is a critical security win.

However, several pressing issues need immediate attention to ensure stability, security, and proper functionality:

Pressing Issues & Suggestions:

Critical: Production Deployment Stability (deploy/argocd/sast-ai-orchestrator-prod-app.yaml)
- Issue: Using targetRevision: HEAD for production ArgoCD applications is highly unstable and non-reproducible.
- Suggestion: Always pin production deployments to an immutable Git tag (e.g., v1.2.3) or a specific commit hash.
- Issue: CreateNamespace=true might bypass production namespace policies.
- Suggestion: Consider pre-provisioning the production namespace (sast-ai-prod) with proper RBAC and quotas.
Critical: Incomplete Production Manifest (deploy/argocd/application-prod.yaml)
- Issue: The application-prod.yaml is incomplete, which will prevent a functional production deployment.
- Suggestion: Complete the application-prod.yaml file, ensuring all production-specific names, namespaces, labels, and value files (e.g., values-prod.yaml) are correctly defined.
Critical: Database Schema Management (deploy/sast-ai-chart/values.yaml)
- Issue: QUARKUS_HIBERNATE_ORM_DATABASE_GENERATION: update is risky for production, potentially leading to unintended schema changes or data loss.
- Suggestion: For production, set this to validate or none and manage database schema migrations using dedicated tools (e.g., Flyway, Liquibase).
Critical: Kubernetes Client TLS Security (deploy/sast-ai-chart/values.yaml)
- Issue: QUARKUS_KUBERNETES_CLIENT_TRUST_CERTS: false disables TLS certificate validation, making the application vulnerable to Man-in-the-Middle attacks.
- Suggestion: Set this to true or provide a strong, well-justified reason for disabling it.
Critical: Image Tagging for Production (implied from values-dev.yaml)
- Issue: While values-dev.yaml uses latest, it's critical that values-prod.yaml does not use latest or any non-immutable tag.
- Suggestion: Ensure app.image.tag in values-prod.yaml specifies an immutable, versioned tag (e.g., v1.0.0).
Minor: Ambiguous Development Image Tag (.github/workflows/build-dev-image.yml)
- Issue: Tagging development builds as latest can be misleading and cause confusion with stable releases.
- Suggestion: Use a more explicit tag like latest-dev or default-branch-dev for development builds.
Minor: ArgoCD Application Cleanup (Makefile)
- Issue: The refactored _clean target removes ArgoCD application cleanup logic.
- Suggestion: Clarify if ArgoCD is still used for deployment. If so, re-introduce the ArgoCD cleanup logic to prevent orphaned application objects.
Minor: Makefile Undefined Variables
- Issue: Variables like DEV_NAMESPACE, PROD_NAMESPACE, and KUBECTL_CMD are used but not defined in the Makefile snippet.
- Suggestion: Define these variables with sensible defaults or clearly document them as required environment variables.
Minor: sed Command Precision (.github/workflows/build-release-image.yml)
- Issue: The sed command uses a broad regex (.*), which might inadvertently update unintended tag: entries.
- Suggestion: Consider using yq for precise YAML updates (e.g., yq e '.image.tag = "$RELEASE_TAG"' -i ...).
Minor: Incomplete Java Code Snippet (src/main/java/com/redhat/sast/api/service/JobService.java)
- Issue: The provided Java diff is incomplete, preventing a proper review.
- Suggestion: Provide the complete method or code block.

Here's my review of the provided diffs:

Review Summary

The pull request refines image tagging and Helm chart version management within GitHub Actions workflows. The changes in build-release-image.yml are a positive step towards clearer separation of production configurations. However, the change in build-dev-image.yml introduces ambiguity regarding the latest tag, which should be carefully considered to avoid potential confusion or misdeployment.

`.github/workflows/build-dev-image.yml`

Issue: Tagging development builds as latest (even when from the default branch) in the build-dev-image.yml workflow can be misleading. latest is conventionally used for the most recent stable or production-ready release. This could lead to confusion or accidental deployment of a non-production quality image if latest is pulled without context.
Suggestion: Revert to using latest-dev or a more explicit tag like default-branch-dev for development builds. This maintains a clear distinction between development and release artifacts.

`.github/workflows/build-release-image.yml`

Positive Note: The shift from updating values.yaml to specifically values-prod.yaml for release builds is an excellent improvement. This clearly separates production configuration, enhances clarity, reduces the risk of misconfiguration, and improves maintainability. The corresponding changes in commit messages and release notes are consistent.
Issue (Minor): The sed command sed -i "s/tag: \.*\/tag: \$RELEASE_TAG\/" deploy/sast-ai-chart/values-prod.yaml uses a broad .* regex. If values-prod.yaml contains multiple tag: entries (e.g., for different components), this could inadvertently update unintended fields.
Suggestion: Consider using a more precise tool like yq to update YAML fields. For example, yq e '.image.tag = "$RELEASE_TAG"' -i deploy/sast-ai-chart/values-prod.yaml (assuming the tag is under image.tag). This provides a more robust and targeted update, reducing the risk of unintended side effects.
Review Summary:

This Pull Request introduces excellent improvements to the project's deployment documentation and configuration. The changes significantly enhance clarity, usability, and maintainability by clearly defining development and production environments, providing explicit deployment commands, and detailing image usage and access instructions. This is a very positive set of changes that will greatly benefit users and maintainers alike. No critical issues or security concerns were found.

Issue Comments per File:

GitHub Actions / Release workflow (implied context)
- Comment: The addition of a 🚀 Deployment section to the release notes is an excellent improvement for clarity. It provides immediate, actionable steps for users on how to deploy to different environments directly from the release information, which is highly beneficial.
README.md
- Comment: This file sees substantial positive changes.
  - The Environment Strategy section is a fantastic addition, clearly outlining the differences between development and production environments (namespaces, image tags, update frequency, logging, and resource limits). This is crucial for proper usage.
  - The Quick Deployment and Environment-Specific Access sections significantly enhance usability and clarity by providing direct, copy-pasteable commands and instructions for accessing deployed applications.
  - Differentiating Container Images and Docker Deployment examples by environment (latest for dev, specific version for prod) is a best practice, promoting more stable deployments in production.
deploy/Makefile
- Comment: Defining DEV_NAMESPACE and PROD_NAMESPACE as explicit variables at the top of the Makefile greatly improves clarity and maintainability. This centralizes environment configuration, making the Makefile easier to understand and future updates simpler. (Assuming the truncated line in the diff completes into a sensible declaration or is removed).
  Review Summary:

This PR introduces a significant and positive refactoring of the Makefile to support environment-specific deployments (development and production). The changes improve clarity, reduce duplication, and enhance maintainability by introducing parameterized internal targets (_deploy, _upgrade) and leveraging environment-specific Helm value files. The use of distinct release names and explicit namespace passing to sub-targets (wait-pods, argocd, show-access) is also a good practice.

Key Strengths:

Reduced Duplication: Excellent use of _deploy and _upgrade targets.
Environment Specificity: Clear separation of deploy-dev and deploy-prod with dedicated value files.
Clarity: Deployment commands are more explicit about the target environment.
Maintainability: Easier to add or modify deployment logic without affecting other environments.

Potential Issues & Suggestions:

`Makefile`

1. Issue: Undefined Variables

Comment: Variables like DEV_NAMESPACE, PROD_NAMESPACE, and KUBECTL_CMD are used but not defined within this provided Makefile snippet.
Suggestion: Ensure these variables are either defined with sensible defaults at the top of the Makefile or clearly documented as required environment variables for the Makefile to function correctly. This improves readability and robustness.

2. Issue: Error Silencing (Minor)

Comment: Commands like helm repo add ... >/dev/null 2>&1 silence all output, including potential errors (e.g., network issues, permission problems).
Suggestion: While common for suppressing "repo already exists" warnings, consider if critical errors should be allowed to surface or be logged in more complex scenarios. For simple Makefiles, this is often accepted, but it's good to be aware of the trade-off.

3. Issue: Inconsistent Messaging (Minor, UX Improvement)

Comment: The use of emojis (🚀, ✅, 🔍, 📦, ⚠️, ⬆️) is a nice touch, but some messages within the _deploy and _upgrade targets lack them or have a slightly different style.
Suggestion: Standardize the use of emojis and messaging for a consistent user experience across all deployment and upgrade steps. For instance, the _deploy target says "Deployment completed!" while deploy-dev says "✅ Deployment completed!".
Review Summary:

The changes primarily focus on refactoring the Makefile's cleanup targets, introducing explicit clean-dev and clean-prod options, and centralizing the cleanup logic into a new internal _clean target. This significantly improves clarity, reduces duplication, and makes the cleanup process more explicit and robust. The messaging has also been improved for better user experience.

The main point of concern is the removal of the ArgoCD application cleanup logic from the refactored _clean target.

File: Makefile

Clarity & Simplicity:
- Improvement: The introduction of .PHONY: clean-dev and .PHONY: clean-prod is a major improvement. It clearly separates development and production cleanup, preventing accidental cleanups and making the intent explicit.
- Improvement: The new _clean target encapsulates the core cleanup logic, reducing duplication and making the Makefile more maintainable. This adheres to the principle of "don't repeat yourself."
- Improvement: Passing NAMESPACE and RELEASE_NAME explicitly to _clean enhances clarity and reusability.
- Improvement: The added emojis (✅, 🧹) and more descriptive echo messages improve the user experience and feedback during execution.
- Minor Improvement: Changing @$(MAKE) wait to @$(MAKE) wait-pods NAMESPACE=$(NAMESPACE) suggests a more specific and explicit waiting mechanism, which is good.
Duplication & Complexity:
- Improvement: The refactoring into _clean effectively eliminates duplication of cleanup commands across clean-dev and clean-prod. This is a very positive change.
- Improvement: The install alias removal simplifies the interface, assuming deploy is now the sole and clear way to install.
Efficiency:
- The cleanup commands (helm uninstall, kubectl delete) are standard and efficient for their purpose. The grep | xargs pattern for force cleanup is a common and effective way to handle potentially lingering resources. The 2>/dev/null || true is good for idempotency and preventing script failure on missing resources.
Specific Issues & Suggestions:
- Issue: Removal of ArgoCD Application Cleanup
  - Context: The original clean target included logic to check for and remove an ArgoCD application ($(RELEASE_NAME)-syncer). The new _clean target (and thus clean-dev, clean-prod) no longer includes this.
  - Problem: If ArgoCD is still part of the deployment pipeline and responsible for managing these applications, removing the Helm release and Kubernetes resources via _clean might leave an orphaned ArgoCD Application object in the cluster, which is not a complete cleanup from ArgoCD's perspective.
  - Suggestion:
    1. Clarify: Confirm if ArgoCD is still used to manage these deployments.
    2. Re-introduce (if applicable): If ArgoCD is still in use, consider re-introducing the ArgoCD cleanup logic. This could be done by:
      - Adding an optional parameter to _clean (e.g., _clean ARGO_CLEANUP=true)
      - Creating a separate _clean-argocd target and calling it from clean-dev/clean-prod if needed (e.g., @$(MAKE) _clean-argocd NAMESPACE=$(NAMESPACE) RELEASE_NAME=$(RELEASE_NAME))
      - Or, if ArgoCD is expected to handle the removal of its Application resource automatically when its managed resources are gone (e.g., via owner references or garbage collection policies), verify this behavior.
- Minor: _clean Target Visibility
  - Suggestion: While the _ prefix convention for internal targets is understood, you might consider adding a comment explicitly stating that _clean is an internal helper target not meant for direct invocation, further enhancing clarity for future maintainers.
    Here's a review of the provided changes:

Review Summary

This PR introduces significant and positive changes aimed at improving the management and clarity of environment-specific deployments, particularly for Development and Production. The adoption of separate ArgoCD application manifests (application-dev.yaml, application-prod.yaml) and corresponding Makefile targets (deploy-dev, deploy-prod, etc.) is a best practice for robust application lifecycle management. The enhanced help target and the new status command in the Makefile greatly improve usability.

Key Improvements:

Environment Isolation: Clear separation of dev and prod deployments, reducing the risk of accidental cross-environment configuration.
Maintainability: Dedicated value files (values-dev.yaml) and specific namespaces for each environment improve maintainability.
Usability: The Makefile has become much more user-friendly with explicit commands for each environment and a helpful status check.

Security Aspects:

Separating deployment configurations for different environments inherently improves security by reducing the blast radius of misconfigurations and enabling stricter access controls per environment.
Explicitly defining namespaces in ArgoCD manifests is a good practice, preventing ambiguous deployments.

The primary concern is an incomplete file in the diff, which needs to be addressed.

File-specific Comments

`Makefile`

Clarity & Simplicity:
- The new status target is very clear and useful for quick checks across environments. The 2>/dev/null || echo " ❌ Not deployed" pattern is excellent for user-friendly error handling.
- The revamped help target, with explicit environment commands and dynamic generation using awk, is a significant improvement in usability and discoverability. It clearly outlines the available actions.
Suggestion:
- Consider a lint or validate target: For a SAST AI project, it might be beneficial to add a target that performs linting or validation of Helm charts or Kubernetes manifests before deployment. This could be useful in CI/CD pipelines as well.

`deploy/argocd/application-dev.yaml` (Renamed from `deploy/argocd/application.yaml`)

Clarity & Simplicity:
- Renaming the file to application-dev.yaml and making names/namespaces explicit (sast-ai-orchestrator-dev, sast-ai-dev) is a fantastic improvement for clarity.
- Adding values-dev.yaml to helm.valueFiles is a crucial best practice for environment-specific configurations.
- Adding the environment: development label is great for organization and filtering within ArgoCD or Kubernetes.
- Fixing the missing newline at the end of the file is a good small cleanup.
No specific issues or suggestions for this file. It looks well-configured for a development environment.

`deploy/argocd/application-prod.yaml`

Critical Issue:
- The provided diff shows this file as incomplete (name: sast-ai-orchestra). It's crucial that this file is fully defined, mirroring the structure of application-dev.yaml but with production-specific values.
- Example missing fields (based on application-dev.yaml):
  - metadata.name: Should be sast-ai-orchestrator-prod.
  - metadata.labels.environment: Should be production.
  - spec.source.helm.releaseName: Should be sast-ai-orchestrator-prod.
  - spec.source.helm.valueFiles: Should include values-prod.yaml (assuming such a file exists or will be created).
  - spec.source.helm.parameters[0].value: Should be sast-ai-prod.
  - spec.destination.namespace: Should be sast-ai-prod.
Suggestion:
- Complete the application-prod.yaml file. Ensure all production-specific names, namespaces, and value files are correctly referenced. This is essential for a functional and distinct production deployment.
  Review Summary:

The pull request introduces ArgoCD application definitions and Helm value overrides for dev and prod environments, which is a good practice for environment separation. However, there are a couple of critical issues related to deployment stability, reproducibility, and security that need to be addressed, especially for the production configuration.

File: deploy/argocd/sast-ai-orchestrator-prod-app.yaml

Issue: targetRevision: HEAD for Production (Critical)
- Comment: Using HEAD as the targetRevision for a production ArgoCD application is a critical issue. It means the application will sync to whatever the HEAD of the specified branch currently is, which can change at any time, leading to non-reproducible and unstable production deployments.
- Suggestion: For production deployments, always pin to an immutable Git tag (e.g., v1.2.3) or a specific commit hash. This ensures that a deployment can always be reproduced exactly as it was at a given point in time.
Suggestion: CreateNamespace=true Policy
- Comment: While CreateNamespace=true is convenient for initial setup, in a well-governed production environment, namespaces are typically pre-provisioned with specific RBAC roles and resource quotas. Relying on ArgoCD to create the namespace might bypass these policies.
- Suggestion: Consider if the sast-ai-prod namespace should be created and managed separately (e.g., via another ArgoCD application for infrastructure or direct kubectl commands by an administrator) before this application is deployed. If CreateNamespace=true is kept, ensure that the default permissions granted to namespaces are acceptable for production.
Positive:
- syncPolicy.automated.prune: false and selfHeal: false for production is a good security and operational practice, ensuring manual review for critical changes.

File: deploy/sast-ai-chart/values-dev.yaml

Issue: Hardcoded Plain-Text Passwords (Security)
- Comment: The PostgreSQL passwords (postgres-dev, quarkus-dev) are hardcoded in plain text directly within the values-dev.yaml file. While this is a development environment, storing sensitive information (even development credentials) in plain text in source control is a significant security risk.
- Suggestion: Utilize Kubernetes Secrets (or a secret management solution like Vault) to manage these credentials. The Helm chart should be configured to reference these secrets rather than embedding the values directly. Even for dev, this helps establish good security hygiene.
Suggestion: image.tag: "latest" and pullPolicy: Always for Development
- Comment: Using latest tag with Always pull policy can lead to non-reproducible development environments, as the image referenced by latest can change frequently. While often tolerated for dev, it can make debugging harder if an issue only appears after an implicit image update.
- Suggestion: If reproducibility is a concern even in dev, consider using more specific tags (e.g., dev-build-123). Otherwise, acknowledge this trade-off for convenience in development. This is acceptable for a dev environment but wouldn't be for staging/prod.

File: deploy/sast-ai-chart/values-prod.yaml (partial content)

Issue: Image Tag for Production (Critical, inferred from values-dev.yaml and common anti-patterns)
- Comment (based on likely content): The values-dev.yaml uses tag: "latest". It is critically important that values-prod.yaml does not use tag: "latest" or any other non-immutable tag. Using latest in production leads to unpredictable deployments, rollbacks, and makes troubleshooting extremely difficult, posing a major stability and security risk.
- Suggestion: Ensure that the app.image.tag in values-prod.yaml specifies an immutable, versioned tag (e.g., v1.0.0, 1.2.3-release) that corresponds to a specific, tested build. The pullPolicy should generally be IfNotPresent or Always with a fixed tag.
  Review Summary:
  The refactoring to separate environment-specific configurations into values-prod.yaml is a good step towards improving clarity and maintainability of the Helm chart. The production-specific settings for replicas, resources, HPA, PDB, and monitoring are generally well-considered. However, there are critical security vulnerabilities related to hardcoded database credentials and potential risks with database schema management and Kubernetes client TLS verification that need immediate attention.

Issue Comments:

File: deploy/sast-ai-chart/values-prod.yaml

Issue: Hardcoded database passwords.
- Comment: CRITICAL SECURITY ISSUE: Hardcoding sensitive credentials like postgresql.auth.postgresPassword and postgresql.auth.password directly in values-prod.yaml is a severe security vulnerability. These values should never be committed to source control.
- Suggestion: Implement Kubernetes Secrets to manage these credentials securely. The Helm chart should then reference these secrets dynamically at deployment time (e.g., using postgresql.auth.existingSecret or postgresql.auth.secretKeys if available in the PostgreSQL sub-chart) instead of defining them explicitly here.
Issue: Generic OpenShift Route Host.
- Comment: The route.host is set to "sast-ai". For production, it's common to use a more specific, fully qualified domain name (FQDN) for external access (e.g., sast-ai.yourcompany.com).
- Suggestion: Confirm if "sast-ai" is an internal-only route or if an external-facing FQDN is required. If external, update the host to an appropriate FQDN.

File: deploy/sast-ai-chart/values.yaml

Issue: QUARKUS_HIBERNATE_ORM_DATABASE_GENERATION: update in shared values.
- Comment: Using update for QUARKUS_HIBERNATE_ORM_DATABASE_GENERATION in a production-bound chart (even if there are overrides, this is the default) is risky. It allows the application to modify the database schema on startup, which can lead to unintended changes or data loss in a production environment.
- Suggestion: For production, it's generally safer to set this to validate or none and manage database schema migrations explicitly using dedicated tools (e.g., Flyway, Liquibase) as part of your CI/CD pipeline.
Issue: QUARKUS_KUBERNETES_CLIENT_TRUST_CERTS: false in shared values.
- Comment: Setting QUARKUS_KUBERNETES_CLIENT_TRUST_CERTS to false means the Kubernetes client will not validate TLS certificates when communicating with the Kubernetes API or other services. This can be a significant security risk, making the application vulnerable to Man-in-the-Middle attacks.
- Suggestion: This setting should generally be true unless there is a very specific, well-justified reason (e.g., properly configured custom CAs not immediately visible here). Please provide justification or consider changing this to true to ensure secure communication.
  Review Summary:

The changes in this pull request are largely positive, primarily focusing on improving the clarity and security posture of the Helm chart configuration. The most significant improvement is the removal of hardcoded PostgreSQL credentials, which is a critical security best practice. The updated comments throughout the values.yaml file also enhance understandability.

The Java code snippet provided is too small and appears incomplete, making a comprehensive review difficult.

File: values.yaml

Issue Comments:

Security Improvement: Removal of Hardcoded Credentials
- Comment: The removal of hardcoded PostgreSQL auth details (password, username, database) is an excellent security enhancement. This forces users to either rely on more secure external database configurations or explicitly provide credentials in a more secure manner (e.g., Kubernetes Secrets), avoiding default, insecure settings.
- Suggestion: Ensure that the chart documentation clearly outlines how users should now provide PostgreSQL credentials, especially if they are deploying a managed PostgreSQL instance via this Helm chart. Reference the externalDatabase options for clarity.
Clarity Improvement: Updated Comments
- Comment: Adding (can be overridden), (shared), (base settings), and (disabled by default) to various configuration sections (e.g., nodeSelector, service, postgresql, hpa, monitoring) significantly improves the clarity and user experience of the Helm chart values. It immediately communicates the purpose and default state of these parameters.

File: src/main/java/com/redhat/sast/api/service/JobService.java

Issue Comments:

Incomplete Code Snippet
- Comment: The provided diff for this Java file is very short and appears incomplete (job.setInputSourceType(InputSourceType.OSH). It's missing a closing parenthesis and semicolon, suggesting either a syntax error in the actual code or an artifact of how the diff was generated/truncated.
- Suggestion: Please provide the complete method or code block where this change occurs for proper review. Without the full context, it's impossible to assess the clarity, correctness, efficiency, or potential security implications of this specific line change.
  Review Summary:

The pull request primarily updates the expected OSH report file names from scan-results-imp.* to scan-results-all.* and removes a GSheetUrl field setting. The changes are straightforward.

Overall Impression:

Clarity & Simplicity: The changes are clear and simple, reflecting an update to external system file naming conventions and removal of what appears to be an unused field.
Efficiency: No efficiency concerns are introduced.
Security: No direct security vulnerabilities are introduced by these changes. The shift in file names is a dependency update; the critical security aspect is ensuring the upstream OSH system is producing the correct and secure reports under the new *-all naming convention.

File: src/main/java/com/redhat/sast/api/service/osh/OshJobCreationService.java

Comment: The removal of job.setGSheetUrl(inputSourceUrl); suggests that Google Sheet URLs are no longer relevant for OSH scan jobs. Please confirm this field is genuinely deprecated or no longer required for OSH jobs to avoid potential future issues if it was used elsewhere.
Comment: Changing the OSH report URL from scan-results-imp.js to scan-results-all.js (and the corresponding Javadoc) implies a breaking change or update in the upstream OSH report generation. It's crucial to confirm this update aligns with the latest OSH service behavior.

File: src/main/resources/application.properties

Comment: The update of osh.json.primary-file from scan-results-imp.json to scan-results-all.json is consistent with the .js file change. This is good for maintaining consistency in file naming conventions across different report types. Ensure the new default filename is correct for the JSON report data.

…setup # Conflicts: # deploy/sast-ai-chart/values.yaml

github-actions

Here's a summary of the provided review, focusing on pressing issues and code suggestions:

Review Summary

The changes introduce significant improvements to CI/CD workflows, Helm chart configuration, and Makefile-based deployment, aiming for better environment separation (dev/prod) and clarity. The refactoring of Makefiles to reduce duplication and the enhanced README.md are positive steps.

However, several critical issues exist, primarily around security, GitOps best practices, and image tagging strategy, which could lead to unstable production deployments, security vulnerabilities, and confusion.

Pressing Issues & Code Suggestions

Critical Security Vulnerability: Hardcoded Passwords in Helm Values
- Issue: values-dev.yaml contains hardcoded postgresql.auth.postgresPassword and quarkus passwords. It's highly likely values-prod.yaml (partially shown) would also hardcode these, creating a severe production security risk.
- Suggestion: Never hardcode credentials in Git. Implement robust secret management using Kubernetes Secrets (e.g., via Sealed Secrets for GitOps), HashiCorp Vault, or other external secret stores. Helm charts should reference these secrets rather than define plain-text values.
Critical GitOps Breach: targetRevision: HEAD for Production ArgoCD Apps
- Issue: argocd/sast-ai-orchestrator-prod.yaml specifies targetRevision: HEAD. This means production will always deploy the latest commit from the main branch, making deployments irreproducible, untraceable, and extremely risky for rollbacks.
- Suggestion: For production ArgoCD Applications, always pin targetRevision to a specific Git tag (e.g., v1.2.3) or a full commit SHA. This ensures immutability, traceability, and stable rollbacks, which are fundamental GitOps principles for production reliability.
Critical Issue: Confusing and Unreliable latest Docker Image Tagging
- Issue: The build-dev-image.yml workflow tags development builds as latest. Simultaneously, the build-release-image.yml workflow removes latest from release notes and doesn't explicitly show applying latest to release images. This results in the latest tag potentially pointing to unstable development builds or having unpredictable behavior, misleading users and risking unstable production deployments if latest is used.
- Suggestion:
  - Establish a clear, single source of truth for latest: Only the build-release-image.yml workflow should ever apply the latest tag, and it should apply it to the latest stable release image.
  - Revert Dev Tagging: build-dev-image.yml should revert to tagging development builds with distinct identifiers like latest-dev, main-latest, or commit SHAs for better traceability.
Missing ArgoCD Cleanup Logic in Makefile
- Issue: The Makefile's new _clean target removes previous ArgoCD-specific cleanup logic (handling application finalizers). If ArgoCD is still used for management, its application resources might be left behind or get stuck.
- Suggestion: Clarify if ArgoCD cleanup is still required. If so, re-introduce a specific ArgoCD cleanup step in _clean or ensure existing mechanisms implicitly handle it.
Incomplete ArgoCD and Makefile Implementations
- Issue: The provided diff for deploy/argocd/application-prod.yaml is incomplete. Additionally, the Makefile's help message lists new environment-specific targets (deploy-dev, clean-prod, etc.), but their full implementations are not included in the provided diffs.
- Suggestion: Provide the complete application-prod.yaml and the full implementation of all advertised Makefile targets for a comprehensive review.

Other Notable Points

Positive Refactoring: The Makefile refactoring using internal targets (_deploy, _clean) and environment-specific wrappers (deploy-dev, clean-prod) is excellent, significantly reducing duplication and improving clarity.
Environment Separation: The use of values-prod.yaml and application-dev.yaml is a good practice for environment-specific configurations.
Documentation Improvement: The README.md enhancements are valuable for user onboarding and understanding deployment strategies.
Minor Issues: Complete the truncated comment in deploy/Makefile, consider a more robust sed regex in build-release-image.yml, and add a newline at the end of values.yaml for best practice.

Okay, I've reviewed the diffs provided. Here are my observations and suggestions:

Review Summary

The changes primarily focus on refining image tagging and Helm chart versioning within CI/CD workflows, especially separating production configurations. The move to use values-prod.yaml for release management is a good practice for environmental separation. However, there's a significant potential issue with the latest Docker image tag strategy that could lead to confusion and deployment of non-release images.

Issue Comments

File: `.github/workflows/build-dev-image.yml`

Issue: Confusing latest Tag for Development Builds

The change reconfigures the default branch's development image to be tagged as latest instead of latest-dev. This is problematic because latest is commonly expected to refer to the latest stable release image. If the release workflow also tags a latest image, there will be a collision or unpredictable behavior where latest could point to either a development build or a release build depending on which workflow ran last. This leads to confusion for consumers and potential deployment of unstable code if latest is used in production. Removing the type=sha tag also reduces traceability for dev builds.

Suggestion:

Revert latest to latest-dev: For clarity and to prevent collisions, development images from the default branch should continue to use a distinct tag like latest-dev or main-latest.
Add back SHA tag (Optional but Recommended): Consider re-including type=sha,prefix={{branch}}- to provide better traceability for development images.
Define a Global latest Strategy: If latest is desired for production, it should only be applied by the release workflow, and ideally, only to the most recent stable release. This needs a clear, consistent strategy across all image-building workflows.

File: `.github/workflows/build-release-image.yml`

Issue 1: Inconsistent latest Tag Management (Critical)

While the build-dev-image.yml now tags latest to the default branch's development build, this build-release-image.yml workflow removes the latest tag from the release notes. If this workflow does not explicitly tag the release image as latest (and the diff provided doesn't show it doing so), then the latest tag will permanently point to the last development build from the default branch. This is a severe problem as users pulling latest will not get the official release, but rather an unverified development version.

Suggestion:

Explicitly Tag Release Images as latest: Add a step in the Build and push Docker image job to apply the latest tag to the release image in addition to the version-specific tag (${{ github.event.release.tag_name }}). This must be done with caution to ensure it doesn't conflict with the build-dev-image.yml latest tag.
- Recommended Strategy: Have build-release-image.yml be the only workflow that manages the latest tag, ensuring latest always points to the latest official release. The build-dev-image.yml should revert its latest tagging.
Update Release Notes: If the release image is tagged latest, update the release notes to accurately reflect this, as latest is a common way for users to pull the most recent stable version.

Issue 2: Sed Command Robustness (Minor)

The sed command sed -i "s/tag: \.*\/tag: \$RELEASE_TAG\/" might be slightly brittle. While it works for tag: some-value, if the tag: line format changes (e.g., includes comments, different spacing, or uses quotes) it might not behave as expected.

Suggestion:

More Specific Regex: Consider making the regex more specific, e.g., s/^$\s*tag:\s*$.*$/\1$RELEASE_TAG/. This anchors the match to the start of the line and preserves leading whitespace, making it more robust against variations in the tag: line itself. However, for simple key-value YAML, the current approach is often sufficient.

Overall, the separation of values-prod.yaml is a positive step. The main focus should be on establishing a clear, unambiguous, and consistent strategy for the latest Docker image tag across all your CI/CD pipelines to prevent security and stability issues for users.
Here's a review of the pull request changes:

Review Summary

This pull request introduces significant improvements to the project's deployment documentation and processes. It clearly defines separate strategies for development and production environments, provides simplified make commands for deployment, and enhances the README.md with critical information on environment strategy, container images, and access. These changes greatly improve clarity, ease of use, and adherence to best practices for environment management.

Issue Comments

File: `release.yml` (or similar GitHub Actions workflow updating release notes)

Observation:
The change here updates a comment within the release description text and adds a new "Deployment" section with instructions. The podman pull commands mentioned are examples provided to the user in the release notes, not commands executed by the workflow itself at this point.

No major issues found. This is a positive change that improves the clarity of release notes for users.

File: `README.md`

Observation:
This file sees substantial improvements, including dedicated sections for "Environment Strategy," "Quick Deployment," "Container Images," and "Environment-Specific Access." The docker build examples are replaced with docker run, which is more practical for quick local testing.

No major issues found. These changes significantly enhance the usability and understanding of the deployment process. The clear distinction between dev and prod environments, along with specific instructions and explanations, is excellent.

File: `deploy/Makefile`

diff --git a/deploy/Makefile b/deploy/Makefile
index 01596c8..206fe8a 100644
--- a/deploy/Makefile
+++ b/deploy/Makefile
@@ -1,9 +1,13 @@
 # Detect OpenShift vs Kubernetes
 KUBECTL_CMD := $(shell command -v oc 2>/dev/null || echo kubectl)
 
-# Get current namespace from kubectl/oc, fallback to sast-ai-workflow
+# Environment-specific settings
+DEV_NAMESPACE := sast-ai-dev
+PROD_NAMESPACE := sast-ai-prod
+
+# Get current namespace from kubectl/oc, fallback to de

Issue: Incomplete Comment
Comment: The comment # Get current namespace from kubectl/oc, fallback to de appears incomplete or truncated. This can lead to confusion regarding the intended fallback behavior for the namespace detection.
Suggestion: Please complete the comment to accurately reflect the fallback logic. For example: # Get current namespace from kubectl/oc, fallback to default or # Get current namespace from kubectl/oc, fallback to $(DEV_NAMESPACE) if that's the intended fallback.
Here's a review of the provided Makefile changes:

Review Summary

The changes introduce a significant improvement in the Makefile's organization and clarity, especially concerning environment-specific deployments. By refactoring deploy and upgrade into internal, reusable targets (_deploy, _upgrade) and creating wrapper targets for dev and prod, the developer has greatly reduced duplication and made the deployment process more robust and easier to manage. The addition of environment-specific VALUES_FILE is also a critical enhancement for configuration management.

Makefile

Issue: Missing Variable Definitions
- Comment: The variables DEV_NAMESPACE and PROD_NAMESPACE are used in the updated NAMESPACE definition and in the new deploy-dev/deploy-prod targets, but their definitions are not provided in this diff.
- Suggestion: Ensure DEV_NAMESPACE and PROD_NAMESPACE are defined at the top of the Makefile or in an included file. For example:
```
DEV_NAMESPACE ?= sast-ai-dev
PROD_NAMESPACE ?= sast-ai-prod
```
  This will make the Makefile self-contained and prevent errors if these variables are not set externally.
Improvement: Environment-specific Targets and Reduced Duplication
- Comment: The refactoring to deploy-dev, deploy-prod, _deploy, upgrade-dev, upgrade-prod, and _upgrade is excellent. It significantly reduces code duplication and clearly separates environment-specific logic. Explicitly passing NAMESPACE, RELEASE_NAME, and VALUES_FILE to the internal targets is a very robust and clear approach.
- Suggestion: None, this is a very good change.
Improvement: Environment-Specific Helm Values
- Comment: Adding -f $(CHART_PATH)/$(VALUES_FILE) allows for specific overrides per environment (values-dev.yaml, values-prod.yaml), which is crucial for managing configuration differences.
- Suggestion: None, this is a great enhancement.
Improvement: Consistent Variable Passing
- Comment: Passing NAMESPACE explicitly to wait-pods, argocd, and show-access ensures these sub-targets operate in the correct context, preventing potential cross-environment issues.
- Suggestion: None, this improves correctness and clarity.
Clarity: Default Namespace Logic
- Comment: The change NAMESPACE ?= $(if $(CURRENT_NAMESPACE),$(CURRENT_NAMESPACE),$(DEV_NAMESPACE)) now correctly defaults to DEV_NAMESPACE if no current Kubernetes namespace is set, aligning with the development-focused default.
- Suggestion: None, this is appropriate for the new structure.
Minor Improvement: Verbose Output
- Comment: The updated @echo messages are more descriptive and include emojis, which slightly enhances the user experience during deployment.
- Suggestion: None, this is a minor but nice touch.
  Here's a review of the provided Makefile changes:

Review Summary

This pull request significantly improves the clarity, maintainability, and reusability of the cleanup operations in the Makefile. The introduction of an internal _clean target to encapsulate the core cleanup logic, coupled with environment-specific clean-dev and clean-prod targets, is an excellent refactoring. It eliminates duplication and makes the cleanup process more explicit and less error-prone. The updates to the upgrade target also add clarity and better feedback.

The main point of discussion revolves around the removal of the ArgoCD specific cleanup logic, which previously handled ArgoCD application finalizers and deletion. Depending on the overall deployment strategy, this might either be a beneficial simplification (if ArgoCD is no longer used for management or has its own cleanup mechanism) or a potential oversight if ArgoCD cleanup is still required.

File: `Makefile`

Improvements:

Consolidated Cleanup Logic (_clean): The creation of the _clean target is a major improvement. It centralizes all the detailed steps for removing a deployment (Helm uninstall, label-based resource deletion, force deletion by name prefix). This significantly reduces duplication and improves maintainability.
Environment-Specific Cleanups (clean-dev, clean-prod): Introducing clean-dev and clean-prod that call the generic _clean target with specific parameters makes the intent clearer and prevents mistakes when cleaning different environments.
Improved upgrade Feedback: Changing @echo "Upgrade completed!" to @echo "✅ Upgrade completed!" adds a nice visual cue for success.
Specific Wait Command: Updating @$(MAKE) wait to @$(MAKE) wait-pods NAMESPACE=$(NAMESPACE) in the upgrade target makes the waiting behavior more explicit and correctly scopes it to the namespace.
Robust Cleanup Commands: The cleanup logic continues to use 2>/dev/null || true and --ignore-not-found=true, which are good practices for idempotent and resilient cleanup scripts, ensuring the process continues even if some resources are already gone.

Potential Issues & Suggestions:

ArgoCD Cleanup Removal:
- Issue: The previous clean target included specific logic to handle ArgoCD Application finalizers and deletion. This entire section has been removed without a clear replacement in the new _clean target. If ArgoCD is still used to manage these deployments, its Application resources might be left behind or become stuck without this specific cleanup.
- Suggestion: Clarify if ArgoCD is still part of the deployment ecosystem.
  - If ArgoCD is no longer used or its cleanup is handled by a separate process, then this removal is justified and simplifies the Makefile.
  - If ArgoCD is still used, consider re-introducing an ArgoCD-specific cleanup target (e.g., _clean-argocd) and calling it appropriately from clean-dev or clean-prod if required. Alternatively, ensure that the general Helm uninstall and Kubernetes resource deletion implicitly handle the ArgoCD Application in your setup.
install Alias Removal:
- Issue: The alias .PHONY: install and install: deploy was removed.
- Suggestion: While deploy is more descriptive, if install was a commonly used target by developers or CI/CD pipelines, its removal might break existing workflows. It might be worthwhile to keep the alias or clearly communicate this change to users of the Makefile.

Verbosity/Clarity in _clean:

Minor Suggestion: The _clean target's initial if helm list... block ends with echo "✅ Deployment removed from $(NAMESPACE)", which comes after the "Cleaning up remaining resources" and "Force cleaning" messages. For better flow, the "Deployment removed!" confirmation could be the final message of the entire _clean target.

--- a/Makefile
+++ b/Makefile
@@ -34,16 +34,18 @@
 _clean: ## Internal clean target
 	@if helm list -n $(NAMESPACE) | grep -q "^$(RELEASE_NAME)"; then \
 		helm uninstall $(RELEASE_NAME) -n $(NAMESPACE) --timeout=$(HELM_TIMEOUT); \
-		@echo "Cleaning up remaining resources by label..."
-		@$(KUBECTL_CMD) delete all,pvc,secrets,configmaps,ingress,routes,networkpolicy,pdb,sa,role,rolebinding,job -n $(NAMESPACE) -l "app.kubernetes.io/instance=$(RELEASE_NAME)" --ignore-not-found=true 2>/dev/null || true
-		@echo "Force cleaning any remaining resources with release name prefix..."
-		@for resource_type in deployment replicaset pod service configmap secret pvc role rolebinding serviceaccount networkpolicy poddisruptionbudget job cronjob ingress route; do \
-			$(KUBECTL_CMD) get $$resource_type -n $(NAMESPACE) -o name 2>/dev/null | grep "$(RELEASE_NAME)" | xargs -r $(KUBECTL_CMD) delete -n $(NAMESPACE) --ignore-not-found=true 2>/dev/null || true; \
-		done
-		@echo "Cleanup completed!"
-		echo "✅ Deployment removed from $(NAMESPACE)"; \
 	else \
 		echo "Release '$(RELEASE_NAME)' not found in namespace '$(NAMESPACE)'"; \
 	fi
+	@echo "Cleaning up remaining resources by label..."
+	@$(KUBECTL_CMD) delete all,pvc,secrets,configmaps,ingress,routes,networkpolicy,pdb,sa,role,rolebinding,job -n $(NAMESPACE) -l "app.kubernetes.io/instance=$(RELEASE_NAME)" --ignore-not-found=true 2>/dev/null || true
+	@echo "Force cleaning any remaining resources with release name prefix..."
+	@for resource_type in deployment replicaset pod service configmap secret pvc role rolebinding serviceaccount networkpolicy poddisruptionbudget job cronjob ingress route; do \
+		$(KUBECTL_CMD) get $$resource_type -n $(NAMESPACE) -o name 2>/dev/null | grep "$(RELEASE_NAME)" | xargs -r $(KUBECTL_CMD) delete -n $(NAMESPACE) --ignore-not-found=true 2>/dev/null || true; \
+	done
+	@echo "Cleanup completed!"
+	@echo "✅ Deployment removed from $(NAMESPACE)"

This modification would move the generic cleanup steps outside the if helm list block, ensuring they run even if Helm didn't find the release, providing a more thorough cleanup attempt regardless.
Review Summary:

The changes introduce a clear separation for development and production deployments using ArgoCD applications and enhance the Makefile with environment-specific helper commands. This is a significant improvement for managing different deployment environments. The explicit naming and configuration for application-dev.yaml are well-executed.

However, there are critical omissions in the provided diff: the actual implementation of the new environment-specific make targets (deploy-dev, clean-prod, etc.) is missing, and the application-prod.yaml file is incomplete.

File: tus

Comment: The status command is a good addition for quick visibility. The expanded help message is much clearer and better organized, especially with the use of awk for dynamic help generation.
Suggestion: The help message lists new environment-specific commands (deploy-dev, deploy-prod, upgrade-dev, upgrade-prod, clean-dev, clean-prod), but their implementations are not provided in this diff. Please ensure these targets are fully implemented and included for review. Without them, the help message is misleading, and core deployment functionality is missing.

File: deploy/argocd/application-dev.yaml

Comment: Renaming the file to application-dev.yaml and explicitly defining development-specific values (e.g., sast-ai-orchestrator-dev for name and releaseName, sast-ai-dev for namespaces, and values-dev.yaml for value files) is a clear and robust approach for environment separation. Using argocd as the metadata namespace for the ArgoCD Application itself is also correct.
Suggestion: None. This change looks good.

File: deploy/argocd/application-prod.yaml

Comment: The file application-prod.yaml is incomplete in the provided diff. It stops at metadata: name: sast-ai-orchestra.
Suggestion: Please provide the full content of application-prod.yaml so it can be properly reviewed. Ensure it mirrors the structure of application-dev.yaml but with production-specific names, namespaces (sast-ai-prod), and value files (values-prod.yaml).
Okay, I've reviewed the initial differences you've provided. Here are my observations and suggestions, keeping in mind clarity, simplicity, efficiency, and security.

Review Summary

The changes introduce ArgoCD Application definitions for production and environment-specific Helm values files for development and production. While the structure is generally good, there are critical security and GitOps best practice concerns, particularly for the production environment. Hardcoded sensitive information, use of HEAD for production deployments, and the latest image tag for development are the primary areas for improvement.

File-specific Comments

`argocd/sast-ai-orchestrator-prod.yaml`

Issue: targetRevision: HEAD for Production
- Problem: Using HEAD for targetRevision in a production ArgoCD Application means it will always deploy the latest commit from the default branch (e.g., main). This introduces significant risk as it lacks reproducibility and makes rollbacks or debugging difficult due to an ever-changing target.
- Suggestion: For production, always pin to a specific Git tag (e.g., v1.2.3) or a full commit SHA. This ensures that the deployed version is immutable, traceable, and easily revertable to a known good state. This is a fundamental GitOps best practice for reliability and safety.
Issue: Redundant Environment Variable Definition
- Problem: The app.env.SAST_AI_WORKFLOW_NAMESPACE is defined as a Helm parameter here (value: sast-ai-prod). Given that values-prod.yaml (and values-dev.yaml) will likely define this, it creates two sources of truth.
- Suggestion: Consolidate environment-specific configurations within the Helm values-prod.yaml file. The ArgoCD Application should primarily specify which values files to use, not override individual parameters unless absolutely necessary (e.g., for unique, dynamic settings not suitable for values files). This simplifies the configuration logic.
Clarification: prune: false and selfHeal: false
- Comment: The comment # Manual sync for production explains the choice. While this is a deliberate decision, it deviates from the typical "self-healing" and "automated reconciliation" aspects of GitOps. Ensure the team understands the implications (e.g., manual cleanup of removed resources, manual remediation of configuration drifts) and that this aligns with the operational strategy for production.

`deploy/sast-ai-chart/values-dev.yaml`

Issue: Hardcoded Passwords (Security Concern)
- Problem: postgresql.auth.postgresPassword: "postgres-dev" and postgresql.auth.password: "quarkus-dev" are hardcoded directly in the values-dev.yaml file. Even for development, hardcoding credentials, especially in a version-controlled repository, is a significant security anti-pattern. It sets a poor precedent and increases the risk of these credentials accidentally being used or exposed in other environments.
- Suggestion:
  1. Use Kubernetes Secrets: For development, credentials should still be managed as Kubernetes Secrets. The Helm chart should define the expectation for these secrets, and they can be created manually or via sealed-secrets for better security in dev.
  2. External Secret Management: For a more robust solution across environments, consider external secret management tools like HashiCorp Vault, AWS Secrets Manager, Azure Key Vault, or GCP Secret Manager.
  3. Dynamic Generation (if ephemeral): If dev databases are ephemeral, consider using a Helm post-install hook or a pre-defined secret that generates a random password if one isn't provided.
Issue: app.image.tag: "latest"
- Problem: While common for development, using the latest tag can lead to non-reproducible deployments even in dev. Developers might inadvertently deploy different image versions without realizing it, leading to "it works on my machine" type issues.
- Suggestion: Encourage the use of more specific development tags, such as dev-branch-name, dev-commit-sha, or a timestamped tag (e.g., YYYYMMDDHHMMSS). This improves traceability and reproducibility, even for development cycles.

`deploy/sast-ai-chart/values-prod.yaml`

Anticipated Issue: Hardcoded Passwords (Critical Security Concern)
- Problem: Based on the values-dev.yaml and the cut-off content, if values-prod.yaml also contains hardcoded passwords for PostgreSQL, this would be a critical security vulnerability for a production environment.
- Suggestion: Absolutely do not hardcode production credentials. Implement robust secret management using Kubernetes Secrets, Sealed Secrets, or external secret stores (Vault, etc.). Production credentials must never reside directly in Git.
Anticipated Issue: app.image.tag: "latest" for Production
- Problem: If values-prod.yaml were to use latest as the image tag (similar to values-dev.yaml), this would be an extremely severe issue for production.
- Suggestion: Production deployments must use immutable and specific image tags (e.g., v1.2.3, commit-sha). This ensures reproducibility, stability, and allows for precise rollbacks.

Review Summary:

The refactoring to introduce values-prod.yaml for environment-specific configurations while keeping common settings in values.yaml is an excellent improvement for clarity, maintainability, and preventing misconfigurations. The production-specific settings like higher replicas, resources, HPA, PDB, and monitoring are well-defined.

However, a critical security vulnerability exists in the new values-prod.yaml regarding hardcoded database credentials.

Issue Comments:

File: deploy/sast-ai-chart/values-prod.yaml

Security: Hardcoded Passwords
- Problem: The postgresql.auth.postgresPassword and postgresql.auth.password fields contain hardcoded plain-text passwords ("postgres-prod-secure", "quarkus-prod-secure"). This is a severe security vulnerability.
- Suggestion: These passwords must not be hardcoded in plain text in a version-controlled YAML file. They should be managed using a Kubernetes Secret, preferably encrypted at rest and dynamically injected or referenced. Consider using tools like Sealed Secrets, Vault, or external secret management systems. The Helm chart should then reference these secrets.

File: deploy/sast-ai-chart/values.yaml

Clarity/Consistency: Default QUARKUS_PROFILE
- Problem: The app.env.QUARKUS_PROFILE is set to prod in the shared values.yaml. While values-prod.yaml correctly overrides this, if a values-dev.yaml also exists (or is planned), it would have to override this explicitly. For a "shared" file, setting a production-specific default might be misleading.
- Suggestion: Consider making the default QUARKUS_PROFILE in values.yaml more generic (e.g., dev if it's the most common local default, or an empty string/placeholder) or removing it entirely from values.yaml to force environment-specific definition. This would make values.yaml truly environment-agnostic for this particular setting.

Review Summary

Overall, this Pull Request introduces positive changes, primarily focusing on improving the clarity of the Helm chart's values.yaml file and significantly enhancing its security posture. The updated comments provide better context, and the removal of default database credentials is a strong security improvement.

File: `values.yaml`

1. Issue: Removal of Default PostgreSQL Credentials (Security Improvement)

Comment: The removal of hardcoded, default PostgreSQL authentication credentials (postgresPassword, username, password, database) is an excellent security enhancement. It eliminates a common vulnerability where charts are deployed with easily guessable default passwords.
Suggestion: To maintain usability, ensure that the chart's README.md or documentation clearly specifies how database credentials should now be provided. This typically involves either using externalDatabase.existingSecret for external databases or providing explicit values for an internal database via --set flags or a custom values.yaml during helm install. If postgresql.enabled: true now relies on the underlying PostgreSQL subchart (e.g., Bitnami) to auto-generate passwords, ensure the application can correctly discover or be configured with those.

2. Issue: Missing Newline at End of File (Best Practice)

Comment: Both the original and modified files are missing a newline character at the end of the file. While not a functional bug, it's a common best practice in many development environments and helps prevent linting warnings or unexpected behavior with some tools.
Suggestion: Add a single blank newline character at the very end of the file.

3. Positive Change: Improved Configuration Section Comments (Clarity)

Comment: The changes to the header comments (e.g., (base settings), (shared), (disabled by default)) significantly improve the clarity and understanding of each configuration section. This makes the values.yaml file easier to navigate and configure for users.

Yael-F · 2025-11-24T08:32:44Z

deploy/sast-ai-chart/values-dev.yaml

+  resources:
+    limits:
+      cpu: 500m
+      memory: 512Mi


The current mem usage of the container is 559.9 MiB, so limiting to 512Mi will cause OOM kill

Signed-off-by: Jude Niroshan <[email protected]>

sonarqubecloud · 2025-11-24T09:12:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions

This Pull Request makes positive strides in structuring the CI/CD pipeline and deployment strategy, particularly with environment-specific configurations using Helm and ArgoCD. The refactoring of the `Makefile` and improved `README.md` are excellent for clarity and maintainability.

However, several critical issues related to security, operational stability, and application resilience need immediate attention.

Pressing Issues & Suggestions:

Critical: Hardcoded Credentials in values-dev.yaml and values-prod.yaml
- Issue: Directly committing sensitive passwords (e.g., postgresql.auth.postgresPassword) in plain text, even for development, is a severe security vulnerability.
- Suggestion: Never store credentials in Git. Use Kubernetes Secrets, Helm Secrets plugin, or a dedicated secret management system (e.g., Vault, AWS Secrets Manager). Parameterize these values so they can be injected securely at deployment time.
- Example (Conceptual for Helm):
```
# In values-*.yaml, reference a secret key
postgresql:
  auth:
    existingSecret: my-db-credentials # Name of pre-created K8s secret
    secretKeys:
      postgresPasswordKey: postgres-password-key # Key within the secret
```
Critical: Production ArgoCD targetRevision: HEAD (application-prod.yaml)
- Issue: Deploying to production using HEAD (the tip of main or default branch) is highly risky. It can lead to unintended deployments of unverified or unstable code.
- Suggestion: For production, always pin to an immutable Git tag (e.g., v1.2.3) or a dedicated, protected release branch (release/v1.2.x). This ensures reproducibility and stability.
Critical: Missing Liveness Probe (values.yaml removal)
- Issue: The liveness probe configuration has been entirely removed. Without it, Kubernetes cannot detect and restart unresponsive application instances, severely impacting service availability and resilience.
- Suggestion: Reintroduce liveness and readiness probes for the application. They are fundamental for robust Kubernetes deployments.
Significant: Ambiguous latest Docker Image Tag Strategy
- Issue: Using latest for dev builds on the default branch can conflict with release latest tags (if used), cause confusion, and reduce traceability for specific dev builds. Release notes also promote latest for production pulls, which is mutable and risky.
- Suggestion:
  - For Dev Images: Define a clear policy. Use distinct tags for dev builds (e.g., snapshot, main-branch-latest). Reintroduce branch-specific SHA tags (type=sha,prefix={{branch}}-) for better traceability.
  - For Release Notes: Guide users to pull the specific, versioned release tag (e.g., v1.0.0), not latest, for production environments.
Significant: Incomplete ArgoCD Cleanup Logic (Makefile)
- Issue: The new _clean target is missing crucial ArgoCD cleanup steps, including patching finalizers on Application resources. This will leave orphaned ArgoCD applications, preventing proper cleanup.
- Suggestion: Reintroduce the specific kubectl commands to delete ArgoCD applications and patch their finalizers within the _clean target, ensuring a complete cleanup.
Significant: Overly Broad Resource Deletion in _clean (Makefile)
- Issue: Using grep "$(RELEASE_NAME)" to delete resources by name is too broad and risky. It could accidentally delete unrelated resources if their names contain the release name as a substring.
- Suggestion: Prioritize label-based deletion (-l "app.kubernetes.io/instance=$(RELEASE_NAME)") as it's more precise. If name-based deletion is truly needed, make the grep more specific (e.g., grep "^$(RELEASE_NAME)-").
Minor: GitHub Actions Version Pinning (actions/upload-artifact v5 to v4)
- Issue: Downgrading an action version without explanation can hide reasons for stability/security.
- Suggestion: Add a concise comment in the workflow explaining the reason for using v4 instead of v5. Otherwise, it's generally best to use the latest stable version.

Positive Changes (Keep up the good work!):

Environmental Separation: Excellent use of values-prod.yaml, values-dev.yaml, application-prod.yaml, and application-dev.yaml.
Makefile Refactoring: _deploy, deploy-dev, deploy-prod, _clean, clean-dev, clean-prod (once fully implemented) are great for clarity and reducing duplication.
Documentation: Enhanced README.md and explicit commit messages significantly improve clarity.
Namespace Awareness: Explicitly passing NAMESPACE to Makefile sub-targets (wait-pods, argocd) improves robustness.
Production Readiness: Enabling HPA, PDB, monitoring, and setting QUARKUS_HIBERNATE_ORM_LOG_SQL: false for production.

Review Summary:
The changes primarily focus on refining the CI/CD pipeline, specifically around Docker image tagging and Helm chart value management for different environments. Introducing values-prod.yaml is a positive step towards clearer environment separation. However, the latest tag strategy for development builds needs careful consideration to avoid ambiguity and potential conflicts with release tags.

File: `.github/workflows/build-dev-image.yml`

Issue: Ambiguous latest tag and reduced traceability.
The change removes latest-dev and branch-specific SHA tags, replacing them with a latest tag applied only on the default branch. This can lead to confusion:

latest tag collision: If latest is also used for stable releases (as implicitly suggested by its removal from the release image workflow's release notes), then dev builds from the default branch might overwrite or conflict with release builds, leading to unpredictable behavior in environments relying on a single latest tag.
Reduced traceability: Removing type=sha,prefix={{branch}}- means it's harder to trace a specific development image back to the exact commit or branch that produced it, especially for non-default branches.

Suggestion:

Clarify latest usage: Define a clear policy for the latest tag. If latest is exclusively for stable releases, then a different tag (e.g., snapshot, main-branch-latest) should be used for dev builds from the default branch. If latest is intended for the latest default branch build, ensure it cannot be confused with or overwrite a production latest tag.
Restore branch-specific tags (optional but recommended): Reconsider adding type=sha,prefix={{branch}}- or a similar mechanism for dev builds to improve traceability and debugging for feature branches. For example, type=sha,value={{branch}}-{{sha}}.

File: `.github/workflows/build-release-image.yml`

Issue: Minor clarity improvement possible for sed command.

Suggestion:

Improve sed regex (Minor): The sed command s/tag: \.*\/tag: \$RELEASE_TAG\// uses \.* which matches zero or more of any character. While it works here, a more specific regex like s/tag: .*/tag: \$RELEASE_TAG/ (matching any characters until the end of the line) or even s/tag: [^[:space:]]*/tag: \$RELEASE_TAG/ (matching non-whitespace characters after tag: ) might be slightly more robust if values-prod.yaml could contain more complex values after tag:. This is a minor point, the current one is likely sufficient.

Positive Changes (No Issues):

Separation of values-prod.yaml: Moving to values-prod.yaml for release builds is an excellent practice. It explicitly separates production-specific configurations from general or development configurations, reducing the risk of accidental deployment of incorrect settings to production.
Explicit commit message: Changing the commit message to "Update production versions..." further clarifies the intent and scope of the changes made by the workflow.
Release notes clarity: Removing the generic latest tag from the release notes and clarifying pull instructions for "production" improves the accuracy and safety of the release documentation. It guides users to explicitly use the versioned tag, which is best practice for production deployments.

Review Summary

Overall, these changes significantly improve the clarity and accessibility of the project's deployment strategy, especially by clearly distinguishing between development and production environments. The additions to the README.md are excellent and provide much-needed guidance for users.

However, there are a couple of minor points related to consistency and best practices, particularly regarding the use of the latest tag in release notes and the versioning of GitHub Actions.

Issue Comments

File: `(release description update snippet)`

Comment:
In the context of generating release notes for a specific version, it's generally best to guide users to pull the specific version that was just released. While mentioning the latest tag for development is useful, including it prominently in the main release notes section can be confusing, as latest is mutable and might not always correspond to the current release after subsequent commits.

Suggestion:
Consider keeping the podman pull command in the release notes focused on pulling the release tag itself. You could perhaps add a separate "For Development" section if you still wish to highlight the latest tag, but the primary instruction should be for the specific version. For example:

podman pull ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ github.event.release.tag_name }}
# For development, you can also pull the latest image:
# podman pull ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:latest

File: `.github/workflows/integration-tests.yml`

Comment:
Downgrading actions/upload-artifact from v5 to v4 without an accompanying comment explaining the reason can be a bit concerning. Newer versions of actions often include bug fixes, performance improvements, and security patches.

Suggestion:
If v5 introduced breaking changes or issues that necessitated reverting to v4, please add a concise comment in the workflow file explaining why v4 is being used instead of the latest stable version. Otherwise, it's generally recommended to use the latest stable version of GitHub Actions.

File: `README.md`

Comment:
The new Deployment section, especially the Environment Strategy, Quick Deployment, and Environment-Specific Access sections, are a significant improvement in clarity and user guidance. This is a very well-executed documentation update.

Suggestion:
No specific issues or suggestions for improvement here. Great job on enhancing the documentation!

Review Summary

The changes in deploy/Makefile represent a significant improvement in clarity, maintainability, and operational safety. By introducing explicit deploy-dev and deploy-prod targets, and consolidating common logic into an internal _deploy target, the developer has greatly reduced duplication and made the deployment process environment-aware. The use of environment-specific values-dev.yaml and values-prod.yaml files is a crucial best practice for Helm deployments. Explicitly passing the NAMESPACE variable to sub-targets (wait-pods, argocd, show-access) enhances robustness and prevents potential misconfigurations.

Issue Comments

deploy/Makefile

Clarity & Maintainability (Positive):
- Comment: The introduction of DEV_NAMESPACE, PROD_NAMESPACE, and dedicated deploy-dev/deploy-prod targets drastically improves the clarity of intent. Refactoring common deployment logic into _deploy is excellent for maintainability and reducing duplication.
- Suggestion: No suggestion, this is a very good change.
Environment Configuration & Security:
- Comment: The use of -f $(CHART_PATH)/$(VALUES_FILE) to apply environment-specific Helm values (values-dev.yaml, values-prod.yaml) is a critical improvement. This ensures that different settings (e.g., resource limits, replica counts, external URLs, security configurations) can be applied correctly per environment, reducing the risk of deploying production settings to dev or vice versa. This significantly enhances operational safety and security posture.
- Suggestion: Ensure values-prod.yaml contains all necessary production-specific overrides and sensitive configurations are handled securely (e.g., via Helm secrets or external secret management systems, not directly in values.yaml if it's committed to Git).
Robustness & Namespace Awareness:
- Comment: Explicitly passing NAMESPACE=$(NAMESPACE) to sub-targets like wait-pods, argocd, and show-access is a very robust change. It ensures that these operations are performed in the intended deployment namespace, rather than relying on the current kubectl context, which could lead to errors or operations in the wrong namespace.
- Suggestion: No suggestion, this is an excellent enhancement.
Variable Usage (Minor Clarity Check):
- Comment: The line --set app.env.SAST_AI_WORKFLOW_NAMESPACE=$(NAMESPACE) sets an application environment variable. While this is likely intended to inform the application of its current namespace, it uses the term SAST_AI_WORKFLOW_NAMESPACE which previously referred to a specific default namespace (sast-ai-workflow). If SAST_AI_WORKFLOW_NAMESPACE is meant to be a fixed, specific identifier independent of where the app is deployed, then dynamically setting it to the current deployment namespace might cause confusion within the application. If it simply means "the namespace where this SAST AI instance is running," then it's fine.
- Suggestion: Confirm with the application's developers that SAST_AI_WORKFLOW_NAMESPACE is indeed expected to reflect the current deployment namespace and not a fixed, logical workflow namespace. If the latter, a different environment variable name might be more appropriate, or the value should be fixed.
Output Messages (Minor):
- Comment: The enhanced output messages with emojis (🚀, ✅, 🔍, 📦, ⚠️) improve the user experience and make the deployment progress more visually appealing and informative.
- Suggestion: None, this is a nice touch.

Review Summary

This Pull Request introduces significant refactoring to the Makefile, primarily around the upgrade and clean operations. The changes aim to improve reusability, clarity, and robustness. The introduction of internal targets like _upgrade and _clean with environment-specific wrappers (clean-dev, clean-prod) is a commendable step towards reducing duplication and making the Makefile more maintainable.

However, there are a few critical points that need attention:

ArgoCD Cleanup Regression: The specific logic for cleaning up ArgoCD applications (including finalizer removal) has been removed from the new _clean target. If ArgoCD is still part of the deployment ecosystem, this is a significant oversight and will lead to hanging or undeletable ArgoCD applications.
Overly Aggressive Cleanup: The new "force cleaning" logic using grep "$(RELEASE_NAME)" for resource names is too broad and carries a risk of accidentally deleting unrelated resources in the same namespace if their names happen to contain the release name as a substring.
Cleanup Scope Change: The additional cleanup steps in _clean (label-based and name-prefix-based deletion) are now nested inside the check for if helm list. This means if a Helm release is already gone (e.g., deleted manually or partially failed), these steps to clean up orphaned resources will no longer execute. The previous clean target executed label-based cleanup unconditionally. This is a behavioral change that could leave more orphaned resources behind.

Overall, the direction of the changes is good, but the identified issues need to be addressed to ensure proper functionality and prevent unintended side effects.

File: `Makefile`

Issue Comments:

ArgoCD Cleanup Missing
- Comment: The clean target previously included specific logic to remove ArgoCD applications and patch finalizers. This crucial step is entirely missing from the new _clean target. If ArgoCD is still used, this will leave orphaned ArgoCD Application resources that may prevent proper cleanup or cause issues.
- Suggestion: Reintroduce the ArgoCD cleanup logic, including the finalizer patch, into the _clean target. Consider making it conditional if ArgoCD usage is not universal across all environments.
Overly Broad Resource Deletion
- Comment: The line $(KUBECTL_CMD) get $$resource_type -n $(NAMESPACE) -o name 2>/dev/null | grep "$(RELEASE_NAME)" | xargs -r $(KUBECTL_CMD) delete -n $(NAMESPACE) --ignore-not-found=true 2>/dev/null || true; uses grep "$(RELEASE_NAME)" to find resources by name. This is too broad and risks deleting resources that are not part of this release but happen to have $(RELEASE_NAME) as a substring in their name. For example, if RELEASE_NAME is myapp and there's an unrelated resource named myapp-database, it could be deleted.
- Suggestion: For deleting orphaned resources, prioritize label-based deletion (-l "app.kubernetes.io/instance=$(RELEASE_NAME)"). If name-based deletion is truly necessary, make it more precise, perhaps by using grep "^$(RELEASE_NAME)-" or grep -E "^$(RELEASE_NAME)(-\w+)*$", or by explicitly listing resource names from a Helm manifest rather than a broad grep. A safer alternative might be to use helm get manifest $(RELEASE_NAME) -n $(NAMESPACE) | kubectl delete -f - but this would require the release to still exist in Helm history.
Cleanup Scope Change (Conditional Execution)
- Comment: The previous clean target had the label-based resource cleanup (delete all,... -l "app.kubernetes.io/instance=$(RELEASE_NAME)") executed unconditionally, meaning it would attempt to clean even if the Helm release wasn't found (useful for orphaned resources). The new _clean target places all additional cleanup steps inside the if helm list ... block. This means if helm list doesn't find the release (e.g., it was partially uninstalled or metadata lost), the subsequent label-based and name-prefix-based cleanup will not run, potentially leaving more orphaned resources.
- Suggestion: Revert the behavior for the general label-based cleanup. Consider moving the kubectl delete ... -l "app.kubernetes.io/instance=$(RELEASE_NAME)" command outside the if helm list ... block within _clean to ensure it always attempts to clean up resources by label, regardless of whether Helm still tracks the release. This provides better idempotency for cleanup.
Clarity & Specificity for wait-pods
- Comment: Renaming wait to wait-pods and passing NAMESPACE=$(NAMESPACE) explicitly is a good improvement for clarity and reusability.
- Suggestion: Ensure the wait-pods target (not shown in this diff) correctly utilizes the passed NAMESPACE variable. (No further action needed if it already does).
  Review Summary:
  The changes represent a positive shift towards a more structured and environment-specific deployment strategy, moving from a generic single-environment setup to distinct development and production environments managed by Helm and ArgoCD. The introduction of specific status and an improved help target in the Makefile enhances operational clarity. The renaming and specialization of the ArgoCD application manifest (application-dev.yaml) is a good step for GitOps best practices.

The main concern is a potential incompleteness regarding the clean targets in the Makefile.

Makefile

Issue:

Missing clean-dev and clean-prod target implementations: The help message lists new clean-dev and clean-prod targets, but their implementations are entirely missing from this diff. The original generic clean target was removed. This leaves a gap in cleanup functionality.
Clarity/Consistency for variables: While DEV_NAMESPACE and PROD_NAMESPACE are introduced in the status target, their definitions are not shown, nor are deploy-dev, deploy-prod etc. which would logically use them. (Assuming these are defined elsewhere in the Makefile, not in this diff.)

Suggestion:

Implement clean-dev and clean-prod targets, preferably using helm uninstall $(RELEASE_NAME)-dev -n $(DEV_NAMESPACE) and helm uninstall $(RELEASE_NAME)-prod -n $(PROD_NAMESPACE) respectively, to align with the Helm-based deployment strategy. Consider if any non-Helm resources need separate cleanup.
Ensure DEV_NAMESPACE, PROD_NAMESPACE, and RELEASE_NAME are clearly defined and configurable at the top of the Makefile or via environment variables for maintainability.

deploy/argocd/application.yaml -> deploy/argocd/application-dev.yaml

No major issues.

Suggestion:

Consistency with Production: Given the rename to application-dev.yaml and the hardcoding of sast-ai-dev and values-dev.yaml, ensure a corresponding application-prod.yaml is created with sast-ai-prod and values-prod.yaml for the production environment to maintain consistency and full multi-environment support.
Namespace for ArgoCD Application: Setting the Application manifest's metadata.namespace to argocd is a good practice as ArgoCD applications typically reside within the ArgoCD control plane's namespace.
Review Summary:

The changes introduce a new ArgoCD application definition for production (application-prod.yaml) and a Helm values override file for development (values-dev.yaml). The overall structure for environment-specific configurations is good, leveraging Helm's value layering and ArgoCD's application model. A minor formatting fix was also applied.

However, there are a few significant concerns, primarily related to security and operational best practices, specifically around hardcoded credentials in development configurations and the use of HEAD for production deployments.

File: `deploy/argocd/application.yaml`

Issue Comments:

Clarity / Best Practice: The change removes \ No newline at end of file.
- Suggestion: This is a good fix. Ensuring files end with a newline character is a standard POSIX practice and improves compatibility with various tools and editors.

File: `deploy/argocd/application-prod.yaml`

Issue Comments:

Security / Operational Best Practice: targetRevision: HEAD is used for the production application.
- Problem: Using HEAD means ArgoCD will deploy whatever is currently at the tip of the default branch (e.g., main). This is highly risky for production as it can lead to unintentional deployments of unverified or unstable code, especially if the default branch is not always pristine.
- Suggestion: For production, it's best practice to pin to a specific, immutable Git tag (e.g., v1.2.3) or a protected branch (e.g., release) that undergoes rigorous testing. This ensures reproducibility and stability. If a branch is used, it should be a dedicated release or production branch that is only updated after thorough validation.
Operational Clarity: The syncPolicy explicitly disables prune and selfHeal with the comment "Manual sync for production".
- Problem: While the comment clarifies intent, this setup means ArgoCD will detect drift but not automatically correct it. This choice is valid for environments requiring strict manual intervention for any production change. However, it's important that this manual intervention process (e.g., how to address drift, how to trigger a sync) is well-defined and understood by the operations team.
- Suggestion: Ensure that the operational procedure for managing production deployments, addressing configuration drift, and performing manual syncs when prune: false and selfHeal: false are in place, is clearly documented.

File: `deploy/sast-ai-chart/values-dev.yaml`

Issue Comments:

Security / Best Practice: Hardcoded postgresql.auth.postgresPassword and postgresql.auth.password.
- Problem: Storing credentials, even for a development environment, directly in version control (especially hardcoded strings) is a significant security anti-pattern. While this is values-dev.yaml, this practice can lead to credentials being accidentally committed to other environments, exposed in logs, or seen by anyone with repository access. It also promotes bad habits.
- Suggestion: For any environment (dev, staging, prod), credentials should be managed securely.
  - Immediate Fix: Use Kubernetes Secrets injected via Helm, or parameterize these values so they can be provided via a secure mechanism (e.g., argocd app create ... --values ... --secret-value-files ...).
  - Longer Term: Integrate with a proper secret management system like HashiCorp Vault, AWS Secrets Manager, Azure Key Vault, or GCP Secret Manager. Helm supports external secrets, and ArgoCD can integrate with these tools. Even for dev, using non-hardcoded, ephemeral secrets generated on deployment is preferable.
Clarity / Reproducibility: app.image.pullPolicy: Always combined with app.image.tag: "latest".
- Problem: While common for development to get the "latest" changes, Always combined with latest can make development environments non-reproducible. If the latest image changes frequently, a redeployment might pull a different image than the previous one, potentially introducing new bugs or unexpected behavior that wasn't present before, making debugging harder.
- Suggestion:
  - If latest genuinely means the most recent passing build from CI/CD and stability is not a primary concern for this specific dev environment, it might be acceptable.
  - However, for better reproducibility, even in dev, consider using a more specific, non-mutable tag (e.g., a Git SHA or a build number). If latest is desired, consider IfNotPresent for pullPolicy to minimize unnecessary pulls and potential for unexpected image changes on every pod restart, though Always is fine if the intent is strictly to get the newest bits on every deploy. Ensure the implications are understood.
    Overall, this PR introduces a good separation of configuration for different environments using Helm's standard values.yaml and environment-specific override files (like values-prod.yaml). This is a recommended practice for managing Helm charts.

The clarity of the comments in both files has improved, clearly stating their purpose. Resource allocations, HPA, PDB, and monitoring are enabled for production, which is good.

However, there is a critical security concern regarding hardcoded secrets.

Review Summary

Strengths:

Improved Configuration Management: Clear separation of default and production-specific configurations using values.yaml and values-prod.yaml.
Production Readiness: Enables HPA, PDB, and monitoring for production, and sets appropriate resource limits and replicas for High Availability.
Performance/Security: QUARKUS_HIBERNATE_ORM_LOG_SQL: false is a critical improvement for both performance and security in production.
Clarity: Excellent comments explaining the purpose of each values file.

Areas for Improvement:

Critical Security Issue: Hardcoded database passwords in values-prod.yaml are a major security vulnerability.

Issue Comments

`deploy/sast-ai-chart/values-prod.yaml`

Issue: Hardcoded Database Passwords
Line: postgresql.auth.postgresPassword: "postgres-prod-secure" and postgresql.auth.password: "quarkus-prod-secure"
Comment:
This is a critical security vulnerability. Passwords for production databases should never be hardcoded in plain text in version control. These values should be managed securely using Kubernetes Secrets, Helm Secrets plugin, HashiCorp Vault, or similar secret management solutions. Storing them in plaintext in Git history makes them easily discoverable and compromises the security of your production environment.

Suggestion:
Refactor these passwords to be injected via Kubernetes Secrets. Helm charts provide mechanisms to reference secrets (e.g., using existingSecret or template functions to read secrets). Ensure these secrets are managed outside of the Git repository for enhanced security. For example:

postgresql:
  auth:
    existingSecret: my-db-credentials-secret # Reference a pre-created Kubernetes Secret
    secretKeys:
      adminPasswordKey: postgres-password
      userPasswordKey: quarkus-password

Or, if dynamically created but still referenced securely:

postgresql:
  auth:
    # Use templating to read from an environment variable or external secret system
    # For example, using a Helm Secrets plugin or externally managed secrets
    postgresPassword: {{ .Values.secrets.postgresPassword | default "CHANGEME" | quote }} # Placeholder/example
    password: {{ .Values.secrets.quarkusPassword | default "CHANGEME" | quote }} # Placeholder/example

The exact implementation depends on your chosen secret management strategy, but the key is to avoid plaintext values in the repository.

`deploy/sast-ai-chart/values.yaml`

Issue: QUARKUS_LOG_LEVEL: INFO as a default
Line: QUARKUS_LOG_LEVEL: INFO
Comment:
While INFO is a sensible default for most environments, if a values-dev.yaml or similar debug-focused environment file is planned, it might be beneficial for QUARKUS_LOG_LEVEL to also be defined per environment. This would allow DEBUG or TRACE logging in development without overriding the default for INFO in the base file.

Suggestion:
Consider moving QUARKUS_LOG_LEVEL to environment-specific values-*.yaml files if development environments are expected to use different log levels (e.g., DEBUG). If INFO is truly the desired baseline for all environments unless explicitly overridden, then the current placement is acceptable.
Review Summary:
The changes primarily focus on improving the clarity of the values.yaml file through more descriptive comments, explicitly marking sections as "shared" or "base settings". A significant positive change from a security perspective is the removal of hardcoded default PostgreSQL credentials. However, the removal of the liveness probe definition is a potential concern for application resilience.

File: values.yaml

Issue 1: Missing Liveness Probe

Problem: The liveness probe configuration for the application (lines 51-53 in the original file) has been completely removed from this diff. A liveness probe is crucial for Kubernetes to determine if an application instance is healthy and to restart it if it becomes unresponsive, ensuring service availability. Without it, a hung application pod will not be automatically recycled, leading to potential service degradation or outage.
Suggestion:
- Clarity: Confirm if the liveness probe has been moved to another section or template within the Helm chart. If so, ensure that movement is tracked and intended.
- Resilience: If it has been removed entirely, it should be re-introduced. It's best practice to always have liveness and readiness probes defined for application deployments.

Issue 2: Removal of Hardcoded PostgreSQL Credentials (Positive Change)

Problem/Improvement: The hardcoded default PostgreSQL credentials (postgresPassword, username, password, database) have been removed. This is an excellent security improvement. Hardcoding sensitive information like passwords in configuration files is a major vulnerability.
Suggestion: Ensure that the chart's templating (or the new externalDatabase configuration) now properly handles how these credentials will be provided. Ideally, this should involve using Kubernetes Secrets for sensitive data, referenced by existingSecret for external databases or generated securely if PostgreSQL is deployed by the chart itself (though this values.yaml suggests externalDatabase is preferred).

Issue 3: Improved Comments and Clarity

Problem/Improvement: Many sections now include "(can be overridden)", "(shared)", or "(base settings)". This significantly improves the clarity and maintainability of the values.yaml, especially in large projects or when this file serves as a base for multiple environments.
Suggestion: No specific suggestion, this is a good change.

Issue 4: Trailing Newline at EOF

Problem/Improvement: The diff fixes the missing newline at the end of the file. This is a minor but good practice for file consistency and compatibility with various tools.
Suggestion: No specific suggestion, this is a good change.

separate dev and prod env setup

eed5156

Signed-off-by: Jude Niroshan <[email protected]>

github-actions bot reviewed Oct 22, 2025

View reviewed changes

Yael-F reviewed Nov 18, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

deploy/argocd/application.yaml Show resolved Hide resolved

deploy/sast-ai-chart/values.yaml Show resolved Hide resolved

JudeNiroshan added 2 commits November 20, 2025 08:20

add missing values files

5c31dd0

Signed-off-by: Jude Niroshan <[email protected]>

merge from origin/main

1b77a69

Signed-off-by: Jude Niroshan <[email protected]>

github-actions bot reviewed Nov 20, 2025

View reviewed changes

document route access

ccffa4d

Signed-off-by: Jude Niroshan <[email protected]>

github-actions bot reviewed Nov 21, 2025

View reviewed changes

add new argocd yaml files

cbb8484

Signed-off-by: Jude Niroshan <[email protected]>

JudeNiroshan requested a review from Yael-F November 21, 2025 13:10

github-actions bot reviewed Nov 21, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into support-new-pipeline-…

036a659

…setup # Conflicts: # deploy/sast-ai-chart/values.yaml

github-actions bot reviewed Nov 24, 2025

View reviewed changes

Yael-F reviewed Nov 24, 2025

View reviewed changes

increase dev resource quota limits

c779397

Signed-off-by: Jude Niroshan <[email protected]>

JudeNiroshan requested a review from Yael-F November 24, 2025 09:12

github-actions bot reviewed Nov 24, 2025

View reviewed changes

Yael-F approved these changes Nov 25, 2025

View reviewed changes

Yael-F merged commit f3fd61b into main Nov 25, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

separate dev and prod env setup #92

separate dev and prod env setup #92

Uh oh!

JudeNiroshan commented Oct 22, 2025 •

edited

Loading

Uh oh!

github-actions bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Uh oh!

github-actions bot left a comment

Uh oh!

github-actions bot left a comment

Uh oh!

github-actions bot left a comment

Uh oh!

Yael-F Nov 24, 2025

Uh oh!

JudeNiroshan Nov 24, 2025

Uh oh!

sonarqubecloud bot commented Nov 24, 2025

Uh oh!

github-actions bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

separate dev and prod env setup #92

separate dev and prod env setup #92

Uh oh!

Conversation

JudeNiroshan commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Pressing Issues & Code Suggestions:

Overall Positive Aspects:

.github/workflows/build-dev-image.yml

.github/workflows/build-release-image.yml

Review Summary

Issues and Suggestions

Review Summary

File: Makefile

Review Summary

Issue Comments per File

Makefile

deploy/argocd/application.yaml (Deleted)

deploy/sast-ai-chart/values-dev.yaml (New File)

deploy/sast-ai-chart/values-dev.yaml

deploy/sast-ai-chart/values-prod.yaml

Review Summary

File Comments

deploy/sast-ai-chart/values.yaml

File: tor

File: src/main/java/com/redhat/sast/api/service/JobService.java

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

File by File Review

GitHub Actions / Release body content (implied context)

README.md

deploy/Makefile

Makefile

Review Summary

Issue Comments

deploy/sast-ai-chart/values-dev.yaml

deploy/sast-ai-chart/values-prod.yaml

Issue Comments:

Review Summary

Issue Comments

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Pressing Issues & Suggestions:

Review Summary

.github/workflows/build-dev-image.yml

.github/workflows/build-release-image.yml

Makefile

Review Summary

File-specific Comments

Makefile

deploy/argocd/application-dev.yaml (Renamed from deploy/argocd/application.yaml)

deploy/argocd/application-prod.yaml

Issue Comments:

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Review Summary

Pressing Issues & Code Suggestions

Other Notable Points

Review Summary

Issue Comments

File: .github/workflows/build-dev-image.yml

File: .github/workflows/build-release-image.yml

Review Summary

Issue Comments

File: release.yml (or similar GitHub Actions workflow updating release notes)

File: README.md

File: deploy/Makefile

JudeNiroshan commented Oct 22, 2025 •

edited

Loading

`.github/workflows/build-dev-image.yml`

`.github/workflows/build-release-image.yml`

File: `Makefile`

`Makefile`

`deploy/argocd/application.yaml` (Deleted)

`deploy/sast-ai-chart/values-dev.yaml` (New File)

`deploy/sast-ai-chart/values-dev.yaml`

`deploy/sast-ai-chart/values-prod.yaml`

`deploy/sast-ai-chart/values.yaml`

File: `tor`

File: `src/main/java/com/redhat/sast/api/service/JobService.java`

`README.md`

`deploy/Makefile`

`Makefile`

`deploy/sast-ai-chart/values-dev.yaml`

`deploy/sast-ai-chart/values-prod.yaml`

`.github/workflows/build-dev-image.yml`

`.github/workflows/build-release-image.yml`

`Makefile`

`Makefile`

`deploy/argocd/application-dev.yaml` (Renamed from `deploy/argocd/application.yaml`)

`deploy/argocd/application-prod.yaml`

File: `.github/workflows/build-dev-image.yml`

File: `.github/workflows/build-release-image.yml`

File: `release.yml` (or similar GitHub Actions workflow updating release notes)

File: `README.md`

File: `deploy/Makefile`

File: `Makefile`

`argocd/sast-ai-orchestrator-prod.yaml`

`deploy/sast-ai-chart/values-dev.yaml`

`deploy/sast-ai-chart/values-prod.yaml`

File: `values.yaml`

File: `.github/workflows/build-dev-image.yml`

File: `.github/workflows/build-release-image.yml`

File: `(release description update snippet)`

File: `.github/workflows/integration-tests.yml`

File: `README.md`

File: `Makefile`

File: `deploy/argocd/application.yaml`

File: `deploy/argocd/application-prod.yaml`

File: `deploy/sast-ai-chart/values-dev.yaml`

`deploy/sast-ai-chart/values-prod.yaml`

`deploy/sast-ai-chart/values.yaml`