Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new storage container for config #232

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Add a new storage container for config #232

merged 3 commits into from
Nov 21, 2024

Conversation

somesylvie
Copy link
Contributor

@somesylvie somesylvie commented Nov 21, 2024

Description

  • Add a new storage container for config and exempt it from the retention policy (config shouldn't be deleted, PHI should)
  • Update the timer trigger function to send a message that matches the config label
  • Create initial config for local

Issue

CDCgov/trusted-intermediary#1082

…on policy

Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: jcrichlake <[email protected]>
Co-Authored-By: James Gilmore <[email protected]>
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Configuration Clarity
The new storage container 'config' is added but not explicitly exempted in the retention policy comments or code. This might lead to confusion or errors in understanding the policy's scope.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add the new config container to the retention policy exemption list

Ensure that the new config_container is explicitly exempted from the retention
policy by adding its prefix to the prefix_match list in the
azurerm_storage_management_policy resource.

operations/template/storage.tf [81]

-prefix_match = ["sftp/", "sftp-dead-letter/"]
+prefix_match = ["sftp/", "sftp-dead-letter/", "config/"]
Suggestion importance[1-10]: 8

Why: The suggestion correctly identifies a potential oversight in the PR where the newly added 'config' container might inadvertently be affected by the retention policy. Adding it to the 'prefix_match' list ensures it is exempt, thus preventing unintended deletions.

8

Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: jcrichlake <[email protected]>
Co-Authored-By: James Gilmore <[email protected]>
…e to match other values

Co-Authored-By: Bella L. Quintero <[email protected]>
Co-Authored-By: jcrichlake <[email protected]>
@somesylvie somesylvie changed the title Add a new storage container for config and exempt it from the retention policy Add a new storage container for config Nov 21, 2024
Copy link
Contributor

@pluckyswan pluckyswan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@somesylvie somesylvie merged commit 510ef3b into main Nov 21, 2024
15 checks passed
@somesylvie somesylvie deleted the initial-config branch November 21, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants