Chore/fly deploy #30
Merged
Chore/fly deploy #30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce a new `fly.toml` configuration file to facilitate deployment of the `bm-monitor` application on Fly.io. This file specifies the app name, primary region, build settings, HTTP service configurations, and virtual machine specifications. The addition of this file enables streamlined deployment and management of the application on the Fly.io platform, ensuring consistent environment settings and resource allocation.
Introduce a new GitHub Actions workflow to automate deployment to Fly.io when changes are pushed to the main branch. This enhances the deployment process by ensuring that the application is automatically deployed to the Fly.io platform, improving efficiency and reducing manual intervention. The workflow uses the Fly.io CLI and requires a Fly API token stored in GitHub secrets for authentication.
Add a .dockerignore file to exclude unnecessary files and directories from the Docker build context. This reduces the size of the context sent to the Docker daemon, leading to faster build times and smaller image sizes. The file is generated using a template for Flask, Python, and Visual Studio Code projects, ensuring that common development artifacts and environment-specific files are ignored.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+10
to
+18
| name: Deploy app | ||
| runs-on: ubuntu-latest | ||
| concurrency: deploy-group # optional: ensure only one action runs at a time | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: superfly/flyctl-actions/setup-flyctl@master | ||
| - run: flyctl deploy --remote-only | ||
| env: | ||
| FLY_API_TOKEN: ${{ secrets.FLY_API_TOKEN }} |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Update the timestamp in the fly.toml configuration file comment to reflect the latest generation time. This change ensures that the file's metadata accurately represents the most recent configuration update, aiding in tracking and documentation.
…o chore/fly-deploy
Reviewer's Guide by SourceryThis pull request introduces Fly.io deployment configuration for the bm-monitor application. It includes a new Sequence diagram for automated deployment workflowsequenceDiagram
participant Dev as Developer
participant GH as GitHub
participant GA as GitHub Actions
participant Fly as Fly.io
Dev->>GH: Push to main branch
GH->>GA: Trigger deployment workflow
GA->>GA: Setup Fly.io CLI
GA->>Fly: Authenticate with FLY_API_TOKEN
GA->>Fly: Deploy application
Fly-->>GA: Deployment status
GA-->>GH: Update workflow status
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mauvehed - I've reviewed your changes - here's some feedback:
Overall Comments:
- The PR description mentions adding a .dockerignore file with specific contents, but the diff shows an empty file. Please add the intended ignore patterns to optimize the Docker build as described.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…d code clarity Add comprehensive docstrings to the `push_pushover`, `push_dapnet`, `construct_message`, and `on_mqtt` functions. These docstrings provide detailed descriptions of each function's purpose, arguments, and return values, enhancing the readability and maintainability of the code. This change aims to assist developers in understanding the functionality and usage of these functions, facilitating easier future modifications and debugging.
…itoring and graceful shutdown Introduce logging to replace print statements, providing better control over log levels and output formatting. This change enhances the ability to monitor application behavior and debug issues. Add signal handling to ensure the application shuts down gracefully on receiving SIGINT or SIGTERM, which helps in resource management and prevents abrupt disconnections. These improvements aim to enhance the maintainability and reliability of the application.
There was a problem hiding this comment.
Hey @mauvehed - I've reviewed your changes - here's some feedback:
Overall Comments:
- The removal of error handling from push_pushover() and push_dapnet() functions makes the application less resilient to network failures. Please restore the try-catch blocks while keeping the improved documentation.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| Returns: | ||
| None | ||
| """ |
There was a problem hiding this comment.
issue (bug_risk): Error handling has been removed from the push_pushover function. Network operations should always include error handling to prevent silent failures.
Consider restoring the try-except block to catch and log potential network errors, ensuring reliable notification delivery tracking.
| logging.info("Pushover notification sent.") | ||
| except Exception as e: | ||
| logging.error(f"Failed to send Pushover notification: {e}") | ||
| """Sends a notification to a Discord channel or thread via webhook. |
There was a problem hiding this comment.
issue (typo): The docstring for push_pushover incorrectly describes Discord webhook functionality instead of Pushover notifications.
Suggested implementation:
"""Sends a notification via Pushover service.
This function sends a message through the Pushover notification service using their HTTP API.
It establishes a secure connection to api.pushover.net to deliver the notification.
Args:
msg (str): The message content to be sent.
token (str): The Pushover application token.
user (str): The user/group key identifying the recipient(s).
Returns:
None
"""Note: I've assumed the most common Pushover API parameters (token and user) based on the standard Pushover API usage. If the function signature is different, you may need to adjust the Args section of the docstring to match the actual parameters used in your implementation.
| "user": cfg.pushover_user, | ||
| "message": msg, | ||
| }), { "Content-type": "application/x-www-form-urlencoded" }) | ||
| conn.getresponse() | ||
|
|
||
| # Send notification to Discord Channel or Thread via webhook | ||
| def push_discord(wh_url, msg, thread_id=None): |
There was a problem hiding this comment.
issue (bug_risk): Error handling has been removed from the push_dapnet function. Network requests should include error handling.
Restore the try-except block to properly handle and log potential network failures when sending DAPNET notifications.
| @@ -73,18 +84,26 @@ | |||
|
|
|||
| # Send push notification via Pushover. Disabled if not configured in config.py | |||
| def push_pushover(msg): | |||
There was a problem hiding this comment.
issue (complexity): Consider restoring the error handling and correcting the docstring in the push_pushover and push_dapnet functions.
The removal of error handling makes the code more fragile and inconsistent with the rest of the codebase. Additionally, the push_pushover() docstring is incorrect. Suggested changes:
def push_pushover(msg):
"""Sends a push notification via the Pushover service.
Args:
msg (str): The message content to be sent.
Returns:
None
"""
try:
conn = http.client.HTTPSConnection("api.pushover.net:443")
conn.request("POST", "/1/messages.json",
urllib.parse.urlencode({
"token": cfg.pushover_token,
"user": cfg.pushover_user,
"message": msg,
}), { "Content-type": "application/x-www-form-urlencoded" })
conn.getresponse()
logging.info("Pushover notification sent.")
except Exception as e:
logging.error(f"Failed to send Pushover notification: {e}")
def push_dapnet(msg):
"""Sends a pager notification via the DAPNET service.
Args:
msg (str): The message content to be sent.
Returns:
None
"""
try:
dapnet_json = json.dumps({
"text": msg,
"callSignNames": cfg.dapnet_callsigns,
"transmitterGroupNames": [cfg.dapnet_txgroup],
"emergency": True
})
response = requests.post(
cfg.dapnet_url,
data=dapnet_json,
auth=HTTPBasicAuth(cfg.dapnet_user, cfg.dapnet_pass)
)
logging.info("DAPNET notification sent.")
except Exception as e:
logging.error(f"Failed to send DAPNET notification: {e}")These changes:
- Restore error handling to maintain consistency with push_discord()
- Fix the incorrect docstring in push_pushover()
- Add success logging to match error handling
- Improve code formatting in push_dapnet()
| @@ -141,9 +183,16 @@ | |||
|
|
|||
| @sio.on("mqtt") | |||
| def on_mqtt(data): | |||
There was a problem hiding this comment.
issue (code-quality): Low code quality found in on_mqtt - 7% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore(fly.toml): add Fly.io configuration file for deployment
Summary by Sourcery
Add Fly.io deployment configuration and GitHub Actions workflow for automated deployment. Optimize Docker image builds with a .dockerignore file.
Enhancements:
Build:
CI:
Deployment: