-
Notifications
You must be signed in to change notification settings - Fork 21
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
ci: add s3 upload action #224
base: master
Are you sure you want to change the base?
Conversation
WalkthroughA new GitHub Actions workflow has been created to automate file uploads to an S3 bucket. The workflow, named "Injective list - Upload Files", is designed to trigger on manual dispatch or pushes to the master branch. It includes conditional execution logic based on commit message content and provides steps for AWS credential configuration and file synchronization. Additionally, a Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/upload.yaml (2)
3-11
: Consider restricting the workflow trigger conditions.The workflow currently triggers on any push to master and allows manual dispatch. Consider:
- Using path filters to trigger only when relevant files change
- Adding environment protection rules for the master branch
Add path filters to make the workflow more focused:
on: workflow_dispatch: push: branches: - master + paths: + - 'json/**'
54-56
: Add error handling and validation for S3 sync.The S3 sync command should include error handling and validation to ensure the operation was successful.
Add error checking and validation:
- name: Sync files to S3 bucket run: | + if [ ! -d "json" ]; then + echo "Error: json directory not found" + exit 1 + fi aws s3 sync json s3://injective-lists/json --delete + if [ $? -ne 0 ]; then + echo "Error: S3 sync failed" + exit 1 + fi🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 56-56: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/upload.yaml
(1 hunks).gitignore
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/upload.yaml
48-48: the runner of "aws-actions/configure-aws-credentials@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.35.1)
.github/workflows/upload.yaml
[warning] 39-39: wrong indentation: expected 10 but found 12
(indentation)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 56-56: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (2)
.github/workflows/upload.yaml (2)
21-27
: Consider removing or documenting the sleep step.The comment indicates uncertainty about the necessity of the sleep step. If this is for rate limiting or dependency purposes, it should be documented clearly. Otherwise, consider removing it to improve workflow execution time.
Could you clarify why this sleep step is necessary? If it's not critical, we should remove it to optimize the workflow execution time.
29-42
: LGTM: Local testing setup is well structured.The conditional installation of dependencies for local testing with ACT is well implemented. The comments clearly explain the purpose of these steps.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 39-39: wrong indentation: expected 10 but found 12
(indentation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/upload.yaml (3)
9-11
: Consider improving job condition readability.The complex condition could be simplified by breaking it into multiple lines and using GitHub's expression syntax more effectively.
jobs: deploy: - if: ${{ github.event_name == 'workflow_dispatch' || contains(github.event.commits[0].message, 'generate json files') && !contains(github.event.commits[0].message, 'skip deploy') }} + if: | + github.event_name == 'workflow_dispatch' || + (contains(github.event.commits[0].message, 'generate json files') && + !contains(github.event.commits[0].message, 'skip deploy'))
31-33
: Improve local testing setup reliability.The current apt-get commands might fail in some environments. Consider using a more robust approach.
- apt-get update && apt-get install sudo -y && apt-get install wget -y && apt-get install unzip -y + apt-get update && \ + DEBIAN_FRONTEND=noninteractive apt-get install -y \ + sudo \ + wget \ + unzip
38-38
: Fix YAML formatting issues.There are several formatting issues in the file:
- Incorrect indentation on line 38
- Trailing spaces on line 42
- Missing newline at end of file
- version: 2 # default + version: 2 # default - + - name: Checkout - aws s3 sync json s3://injective-lists/json --delete + aws s3 sync json s3://injective-lists/json --delete +Also applies to: 42-42, 55-55
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 38-38: wrong indentation: expected 10 but found 12
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/upload.yaml
(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/upload.yaml
[warning] 38-38: wrong indentation: expected 10 but found 12
(indentation)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 55-55: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/upload.yaml (1)
20-25
: Verify the necessity of the sleep step.The comment indicates uncertainty about the sleep step's necessity. If this is for rate limiting or dependency purposes, consider:
- Documenting the specific reason for the delay
- Reducing the sleep duration if possible
- Removing it if not required
✅ Verification successful
Sleep step appears to be a legitimate deployment pattern
The 60-second sleep step is consistently implemented in both the upload and deploy workflows, suggesting it serves a specific purpose in the deployment process. While the exact reason isn't documented, its presence in both workflows indicates it's an intentional pattern rather than an unnecessary addition.
Recommendation:
- Document the specific reason for this delay in both workflow files to prevent future confusion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any rate limits or dependencies in the codebase that might require this delay rg -i "rate limit|throttle|delay" .Length of output: 1654
Script:
#!/bin/bash # Find deploy script and other workflow files with sleep actions echo "=== Searching for deploy script ===" fd deploy -t f echo -e "\n=== Checking workflows for sleep patterns ===" rg -i "sleep|wait" .github/workflows/ echo -e "\n=== Checking deploy script content if found ===" fd deploy -t f -x cat {}Length of output: 654
Summary by CodeRabbit
New Features
Chores
.gitignore
to exclude.secrets
files.