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

Feat/change to s3 #175

Merged
merged 4 commits into from
Oct 20, 2024
Merged

Feat/change to s3 #175

merged 4 commits into from
Oct 20, 2024

Conversation

NichArchA82
Copy link
Contributor

No description provided.

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The PR introduces new environment variables for AWS credentials (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY). While these are using GitHub secrets, which is a good practice, ensure that these secrets are properly managed and rotated regularly to minimize the risk of exposure.

⚡ Recommended focus areas for review

Configuration Change
The PR changes the storage solution from R2 to S3. Verify that all necessary S3 configurations are correctly set up and that the S3 bucket has appropriate permissions.

Error Handling
The new S3 copy command doesn't include any error handling or logging. Consider adding error checks and logging for better debugging and reliability.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Use official AWS actions for improved security and maintainability

Consider using the official AWS CLI GitHub Action instead of a third-party action
for installing the AWS CLI. This can improve security and ensure you're using the
most up-to-date and supported method.

.github/workflows/packer-qemu.yml [45-50]

-- id: install-aws-cli
-  uses: unfor19/install-aws-cli-action@v1
+- name: Configure AWS credentials
+  uses: aws-actions/configure-aws-credentials@v1
   with:
-    version: 2
-    verbose: false
-    arch: amd64
+    aws-access-key-id: ${{ secrets.S3_ACCESS_KEY_ID }}
+    aws-secret-access-key: ${{ secrets.S3_SECRET_ACCESS_KEY }}
+    aws-region: ${{ secrets.S3_BUCKET_REGION }}
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Security
Improve security by using pre-configured AWS credentials

Instead of setting AWS credentials as environment variables in the 'copy file to s3
storage' step, consider using the AWS credentials set up by the official AWS CLI
action. This approach is more secure and follows best practices for handling
credentials in GitHub Actions.

.github/workflows/packer-qemu.yml [52-58]

-- name: copy file to s3 storage
+- name: Copy file to S3 storage
   run: |
     aws s3 cp images/${{ github.event.workflow_run.head_branch }}.qcow2 s3://${{ secrets.S3_BUCKET }}/${{ github.event.workflow_run.head_branch }}.qcow2
-  env:
-    AWS_ACCESS_KEY_ID: ${{ secrets.S3_ACCESS_KEY_ID }}
-    AWS_SECRET_ACCESS_KEY: ${{ secrets.S3_SECRET_ACCESS_KEY }}
-    AWS_DEFAULT_REGION: ${{ secrets.S3_BUCKET_REGION }}
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7
Enhancement
Add error handling for S3 file upload to improve reliability

Add error handling to the AWS S3 copy command to ensure the workflow fails if the
file upload is unsuccessful. This can be done by adding the '--no-progress' flag and
checking the exit code.

.github/workflows/packer-qemu.yml [52-54]

-- name: copy file to s3 storage
+- name: Copy file to S3 storage
   run: |
-    aws s3 cp images/${{ github.event.workflow_run.head_branch }}.qcow2 s3://${{ secrets.S3_BUCKET }}/${{ github.event.workflow_run.head_branch }}.qcow2
+    aws s3 cp --no-progress images/${{ github.event.workflow_run.head_branch }}.qcow2 s3://${{ secrets.S3_BUCKET }}/${{ github.event.workflow_run.head_branch }}.qcow2
+    if [ $? -ne 0 ]; then
+      echo "Failed to upload file to S3"
+      exit 1
+    fi
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why:

7

💡 Need additional feedback ? start a PR chat

@NichArchA82 NichArchA82 merged commit d4aeb2a into main Oct 20, 2024
3 checks passed
@NichArchA82 NichArchA82 deleted the feat/change-to-s3 branch October 20, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants