Add support for partition-tables. #67
Conversation
WalkthroughGroovy, baby! This pull request introduces a fab new input parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant Release Job
participant Artifact Storage
User->>GitHub Actions: Trigger workflow with build-tables
GitHub Actions->>Release Job: Start release-partition-tables job
Release Job->>Release Job: Checkout repository
Release Job->>Release Job: Prepare partition tables
Release Job->>Artifact Storage: Release artifacts
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
README.md (1)
26-28: Shagadelic new partition tables section!The addition of the partition tables section with a reference to its dedicated README is absolutely fab! However, let's make it even more groovy, baby!
Consider adding a brief one-liner explaining what partition tables are, like this:
See the [partition-tables/README.md](partition-tables/README.md) for a list of the most commonly used partition tables. You can also browse -the `partition-tables` directory for more partition tables. +the `partition-tables` directory for more partition tables. Partition +tables define how the flash memory is organized on ESP32 devices.variants/Contributing.md (6)
3-8: Yeah baby, yeah! Let's add more specifics about variant acceptance criteria!Groovy introduction, but it would be totally shagadelic if we could specify:
- What makes a variant "generally useful"
- How often the automated builds run
- Where to find the built variants
12-14: Oh behave! Let's add some smashing version control guidelines!The overview's got the mojo, baby, but it would be even more groovy with:
- Git branching strategy recommendations
- Testing requirements before submission
- Naming conventions for variants
18-20: Smashing example, but let's add some punctuation, baby!A comma is missing after
esp32-ota-1c0000. Also, it would be absolutely fab to add:
- Minimum/maximum partition size constraints
- Impact on flash memory layout
-the `esp32-ota-1c0000` is an example of this where +the `esp32-ota-1c0000` is an example of this, where🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: Possible missing comma found.
Context: ...Theesp32-ota-1c0000is an example of this where the OTA partition size has been i...(AI_HYDRA_LEO_MISSING_COMMA)
34-41: Crikey! Let's make this code block groovy with proper formatting!The diff command is looking sharp, but let's make it even more shagadelic:
- Add 'bash' language specification to the code block
- Consider providing a helper script to generate patches
- ``` + ```bash🧰 Tools
🪛 Markdownlint
34-34: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
48-51: Let's make this section totally Austin-worthy with more details!The section's a bit light, baby. It would be smashing to add:
- Example of creating a recursive diff
- Patch naming conventions
- Common gotchas when modifying the main directory
1-2: Yeah baby! Let's add a table of contents to this groovy document!For a document this size, a table of contents would make it absolutely smashing to navigate!
# Contributing ## Table of Contents - [Overview](#overview) - [Creating a variant](#creating-a-variant) - [Partition changes](#partition-changes) - [sdkconfig changes](#sdkconfig-changes) - [Main changes](#main-changes)🧰 Tools
🪛 LanguageTool
[style] ~2-~2: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: # Contributing Feel free to open issues and pull requests with new ...(FEEL_FREE_TO_STYLE_ME)
.github/workflows/build.yml (1)
396-398: Let's fix this groovy shell script, shall we?There's a potential word splitting issue in the environment variable assignment.
Here's the fix, baby:
- echo "TOIT_GIT_VERSION=${{ github.event.inputs.toit-version }}" >> $GITHUB_ENV + echo "TOIT_GIT_VERSION='${{ github.event.inputs.toit-version }}'" >> "$GITHUB_ENV"🧰 Tools
🪛 actionlint
397-397: shellcheck reported issue in this script: SC2086:info:1:68: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
partition-tables/ota-1c0000/partitions.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
.github/workflows/build.yml(4 hunks)README.md(2 hunks)partition-tables/README.md(1 hunks)partition-tables/ota-1c0000/README.md(1 hunks)variants/Contributing.md(1 hunks)variants/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- partition-tables/README.md
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yml
397-397: shellcheck reported issue in this script: SC2086:info:1:68: Double quote to prevent globbing and word splitting
(shellcheck)
401-401: shellcheck reported issue in this script: SC2045:error:2:16: Iterating over ls output is fragile. Use globs
(shellcheck)
🪛 LanguageTool
partition-tables/ota-1c0000/README.md
[typographical] ~6-~6: Consider adding a comma after this introductory phrase.
Context: ...ions to 0x1C0000 (1835008) bytes. As a consequence the size of the programs partition is s...
(AS_A_NN_COMMA)
variants/Contributing.md
[style] ~2-~2: The phrase ‘feel free to’ is used quite frequently. Consider using a less frequent alternative to set your writing apart from others and make it sound more professional.
Context: # Contributing Feel free to open issues and pull requests with new ...
(FEEL_FREE_TO_STYLE_ME)
[uncategorized] ~19-~19: Possible missing comma found.
Context: ...The esp32-ota-1c0000 is an example of this where the OTA partition size has been i...
(AI_HYDRA_LEO_MISSING_COMMA)
variants/README.md
[uncategorized] ~18-~18: A comma may be missing after the conjunctive/linking adverb ‘Otherwise’.
Context: ...n ESP32 variant for boards with SPIRAM. Otherwise the same as the ESP32 variant. ## esp3...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 Markdownlint
variants/Contributing.md
34-34: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
README.md (2)
3-3: Yeah baby, yeah! The title update is groovy!
The new title accurately reflects the expanded scope of the repository to include partition tables alongside envelopes. Smashing documentation update, baby!
22-24: Oh behave! Brilliant restructuring of variants documentation!
The new reference to variants/README.md is a smashing way to organize the documentation, baby! It keeps the main README clean while ensuring all the juicy details about variants are just a click away.
.github/workflows/build.yml (1)
26-30: Yeah baby, yeah! The new input parameter looks groovy!
The build-tables parameter is well-structured and follows the existing pattern of the workflow inputs. Smashing!
.github/workflows/build.yml
Outdated
| @@ -192,7 +197,7 @@ jobs: | |||
| artifacts: | | |||
| envelopes/* | |||
| body: | | |||
| Envelopes for Toit SDK ${{ github.event.inputs.toit-version }} | |||
| Envelopes and partition tables for Toit SDK ${{ github.event.inputs.toit-version }} | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hold on a tick, baby! Let's make the release message more dynamic!
The release message mentions partition tables even when build-tables is false. That's not very groovy, is it?
Let's make it more shagadelic with a conditional message:
- Envelopes and partition tables for Toit SDK ${{ github.event.inputs.toit-version }}
+ ${{ github.event.inputs.build-tables == 'true' && 'Envelopes and partition tables' || 'Envelopes' }} for Toit SDK ${{ github.event.inputs.toit-version }}Also applies to: 308-308, 379-379
.github/workflows/build.yml
Outdated
| for variant in $(ls partition-tables); do | ||
| if [ -e "partition-tables/$variant/partitions.csv" ]; then | ||
| cp "partition-tables/$variant/partitions.csv" "build/partition-table-$variant.csv" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Crikey! Let's make this shell script more bulletproof, baby!
Using ls for iteration is a bit dodgy. Also, we should add some validation, yeah?
Here's a more robust implementation:
- for variant in $(ls partition-tables); do
- if [ -e "partition-tables/$variant/partitions.csv" ]; then
- cp "partition-tables/$variant/partitions.csv" "build/partition-table-$variant.csv"
- fi
+ shopt -s nullglob
+ for csv_file in partition-tables/*/partitions.csv; do
+ variant=$(basename "$(dirname "$csv_file")")
+ # Validate CSV format
+ if head -n1 "$csv_file" | grep -q "^Name,Type,SubType,Offset,Size,Flags"; then
+ cp "$csv_file" "build/partition-table-$variant.csv"
+ else
+ echo "Warning: Invalid partition table format in $csv_file"
+ exit 1
+ fi
+ done
+ if [ ! "$(ls -A build/)" ]; then
+ echo "Error: No valid partition tables found!"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for variant in $(ls partition-tables); do | |
| if [ -e "partition-tables/$variant/partitions.csv" ]; then | |
| cp "partition-tables/$variant/partitions.csv" "build/partition-table-$variant.csv" | |
| fi | |
| done | |
| shopt -s nullglob | |
| for csv_file in partition-tables/*/partitions.csv; do | |
| variant=$(basename "$(dirname "$csv_file")") | |
| # Validate CSV format | |
| if head -n1 "$csv_file" | grep -q "^Name,Type,SubType,Offset,Size,Flags"; then | |
| cp "$csv_file" "build/partition-table-$variant.csv" | |
| else | |
| echo "Warning: Invalid partition table format in $csv_file" | |
| exit 1 | |
| fi | |
| done | |
| if [ ! "$(ls -A build/)" ]; then | |
| echo "Error: No valid partition tables found!" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/build.yml(4 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yml
397-397: shellcheck reported issue in this script: SC2086:info:1:68: Double quote to prevent globbing and word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2046:warning:4:22: Quote this to prevent word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/build.yml (3)
26-30: Yeah baby, yeah! The new input parameter looks groovy!
The build-tables parameter is well-integrated with the existing workflow inputs, maintaining backward compatibility with its default value of true. Smashing!
200-200: Past review comment is still valid, baby!
Also applies to: 308-308, 379-379, 419-419
385-411:
Behave, shell script! Let's make you more secure, yeah!
The script needs some groovy shell quoting to prevent any funky word splitting or globbing, baby!
Apply these changes to make it more shagadelic:
- variant=$(basename $(dirname $csv_file))
- if [ -e "partition-tables/$variant/partitions.csv" ]; then
+ variant=$(basename "$(dirname "$csv_file")")
+ if [ -e "partition-tables/${variant}/partitions.csv" ]; then🧰 Tools
🪛 actionlint
397-397: shellcheck reported issue in this script: SC2086:info:1:68: Double quote to prevent globbing and word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2046:warning:4:22: Quote this to prevent word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/build.yml(4 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/build.yml
397-397: shellcheck reported issue in this script: SC2086:info:1:68: Double quote to prevent globbing and word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2046:warning:4:22: Quote this to prevent word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/build.yml (3)
26-30: Yeah baby, yeah! The new input parameter looks groovy!
The build-tables parameter is well-placed with other build parameters and follows the same pattern. Smashing!
200-200: Oh behave! The release message needs to be more dynamic, baby!
A past review comment already suggested making the release message conditional based on build-tables. That suggestion is still valid and groovy!
Also applies to: 308-308, 379-379
400-417: Yeah baby! Let's make this script totally awesome!
Two past review comments are still valid and groovy:
- Making the shell script more bulletproof with validation
- Being more specific with artifact patterns
These suggestions would make the code even more shagadelic!
🧰 Tools
🪛 actionlint
402-402: shellcheck reported issue in this script: SC2046:warning:4:22: Quote this to prevent word splitting
(shellcheck)
402-402: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
.github/workflows/build.yml
Outdated
| @@ -23,6 +23,11 @@ on: | |||
| required: false | |||
| type: boolean | |||
| default: true | |||
| build-tables: | |||
.github/workflows/build.yml
Outdated
| @@ -23,6 +23,11 @@ on: | |||
| required: false | |||
| type: boolean | |||
| default: true | |||
| build-tables: | |||
| description: Build partition tables | |||
.github/workflows/build.yml
Outdated
| @@ -192,7 +197,7 @@ jobs: | |||
| artifacts: | | |||
| envelopes/* | |||
| body: | | |||
| Envelopes for Toit SDK ${{ github.event.inputs.toit-version }} | |||
| Envelopes and partition tables for Toit SDK ${{ github.event.inputs.toit-version }} | |||
.github/workflows/build.yml
Outdated
| run: | | ||
| mkdir -p build | ||
| shopt -s nullglob | ||
| for csv_file in partition-tables/*/partitions.csv; do |
There was a problem hiding this comment.
Let's put these in partitions//partitions.csv - maybe even esp32/partitions//partitions.cvs.
…nto floitsch/partitions
Summary by CodeRabbit
Release Notes
New Features
Documentation