-
-
Notifications
You must be signed in to change notification settings - Fork 12
Add OurAirports sync script #49
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
base: master
Are you sure you want to change the base?
Conversation
convert csvs from https://ourairports.com/data/ to json
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a GitHub Actions workflow that runs a new Ruby script to fetch airport and frequency CSVs, convert them to JSON, and commit the generated JSON files on a schedule, manual dispatch, and non-PR triggers. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as GitHub Actions (cron / dispatch)
participant Repo as Repository (workflow)
participant Runner as Runner (ubuntu-latest)
participant Remote as OurAirports CSV Host
participant Commit as Git Commit Action
Scheduler->>Repo: trigger workflow (cron / dispatch / PR)
Repo->>Runner: start job
Runner->>Runner: checkout code\nsetup Ruby\ninstall deps
Runner->>Remote: HTTP GET airports.csv
Remote-->>Runner: airports CSV
Runner->>Remote: HTTP GET frequencies.csv
Remote-->>Runner: frequencies CSV
Runner->>Runner: parse CSV -> build JSON files
Runner->>Commit: run EndBug/add-and-commit (if not PR)
Commit-->>Repo: commit JSON files (if changes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Pull request overview
This PR adds a Ruby script to synchronize airport and frequency data from OurAirports.com, converting CSV data to JSON format with scheduled automated updates.
Changes:
- Creates a Ruby script that downloads airport and frequency CSVs from OurAirports, enriches the data with ISO country/region information, and outputs both compact and pretty-printed JSON files
- Adds a GitHub Actions workflow to run the sync script daily at 13:00 UTC and automatically commit the updated JSON files
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/ourairports | Ruby script that fetches CSV data from OurAirports, maps fields to JSON structure with country enrichment, and writes output files |
| .github/workflows/update-ourairports.yml | GitHub Actions workflow configuration for scheduled execution and automated commits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| :ourairports_airport_id => row['airport_ref'].to_i, | ||
| :our_airports_id => row['id'].to_i, |
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.
these were strings originally, making ints to be consistent
| }, | ||
| :ident => row['ident'], | ||
| :municipality => row['municipality'], | ||
| :icao_code => row['icao_code'] || row['local_code'] || row['ident'], |
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.
I don't know if this is right
|
|
||
| { | ||
| :region => region_name, | ||
| :faa_lid => row['local_code'] || row['ident'], |
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.
this looks right from what i can tell
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
🤖 Fix all issues with AI agents
In @.github/workflows/update-ourairports.yml:
- Line 37: The glob pattern under the workflow's asset add step is wrong: change
the "add: \"*.*json\"" entry to match the generated files (e.g., use "add:
\"json/*.json\"" or "add: \"json/**/*.json\"" so json/airports.json and
json/airport-frequencies.json are included); update the add pattern in the same
workflow step where "add: \"*.*json\"" appears.
🧹 Nitpick comments (6)
scripts/ourairports (4)
19-23: Add error handling for HTTP requests.
Net::HTTP.getdoesn't handle redirects or raise on HTTP errors. If the remote server is unavailable or returns an error status, the script will silently produce invalid output.♻️ Suggested improvement with error handling
def download_csv(url) uri = URI(url) - response = Net::HTTP.get(uri) - response + response = Net::HTTP.get_response(uri) + case response + when Net::HTTPSuccess + response.body + when Net::HTTPRedirection + download_csv(response['location']) + else + raise "Failed to download #{url}: #{response.code} #{response.message}" + end end
37-37: Handle nil values in location string.If
municipalityis nil or empty, the location string will produce results like, Region, Country. Consider filtering out nil/empty values.♻️ Suggested fix
- :location => "#{row['municipality']}, #{region_name}, #{country_name}", + :location => [row['municipality'], region_name, country_name].compact.reject(&:empty?).join(', '),
49-51: Redundant.to_hcalls.Hash literals are already hashes; calling
.to_hon them is unnecessary.♻️ Suggested fix
- }.to_h + } end - { :airports => airports }.to_h + { :airports => airports } end
54-66: Consider convertingfrequency_mhzto a numeric type.Other numeric fields like
ourairports_airport_idandour_airports_idare converted with.to_i, butfrequency_mhzremains a string. For consistency and easier downstream use, consider converting it to a float.♻️ Suggested fix
- :frequency_mhz => row['frequency_mhz'], + :frequency_mhz => row['frequency_mhz'].to_f,.github/workflows/update-ourairports.yml (2)
19-21: Job nametestis misleading.The job performs a sync/update operation, not testing. Consider renaming to
syncorupdate-airportsfor clarity.♻️ Suggested fix
jobs: - test: + sync: runs-on: ubuntu-latest
24-29: Consider pinning the Ruby version.
ruby/setup-ruby@v1without aruby-versioninput uses the default, which could change over time and cause unexpected behavior.♻️ Suggested improvement
- name: Set up Ruby uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.2'
convert csvs from https://ourairports.com/data/ to json
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.