-
Notifications
You must be signed in to change notification settings - Fork 4
Added maintenance guide #393
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
Summary of ChangesHello @alirana01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical maintenance guide for the ESP-IDF Installation Manager (EIM). The guide centralizes comprehensive information on how to maintain EIM's diverse distribution channels, ensuring consistency, security, and functionality across various platforms and package managers. It aims to streamline future maintenance efforts by providing clear procedures and references for all external components and workflows involved in EIM's release process, thereby enhancing the long-term sustainability and reliability of the project. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a comprehensive maintenance guide for the EIM project, which is a valuable addition for future maintainers. The guide is well-structured and detailed. My review focuses on improving its long-term maintainability and ensuring all information is accurate and points to official resources.
Key feedback points include:
- Removing hardcoded line numbers which can become outdated.
- Replacing references to personal repositories with official ones.
- Ensuring consistency in examples.
- Aligning the documented components with the actual source code implementation.
Overall, this is a great initiative. Addressing these points will make the guide more robust and professional.
| **URL:** https://github.com/Hahihula/eim-idf-build-docker | ||
|
|
||
| **Docker Hub:** https://hub.docker.com/r/hahihula/eim-idf-build |
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 section, and others throughout the document (e.g., Docker Hub link, WinGet fork), refer to repositories and artifacts under the personal user Hahihula. For an official maintenance guide, all resources should point to official espressif organization repositories or assets. Using personal repositories introduces a single point of failure and potential security concerns, especially as the PR description notes some of this was for personal understanding.
Please replace all instances of Hahihula with the appropriate espressif organization-owned repository or a clear placeholder like espressif/<repo-name>. This applies to repository URLs, Docker image names, and fork user configurations.
Example occurrences:
- Line 280:
https://github.com/Hahihula/eim-idf-build-docker - Line 282:
https://hub.docker.com/r/hahihula/eim-idf-build - Line 968:
Hahihula/winget-pkgs - Line 1190:
https://github.com/Hahihula/eim-idf-build-docker
| | File | Description | Lines | | ||
| |------|-------------|-------| | ||
| | `src-tauri/scoop_manifest_templates/7zip.json` | 7-Zip archiver (v25.01) | 73 | | ||
| | `src-tauri/scoop_manifest_templates/git.json` | Git for Windows (v2.50.1) | 82 | | ||
| | `src-tauri/scoop_manifest_templates/python311.json` | Python 3.11.9 | 95 | | ||
| | `src-tauri/scoop_manifest_templates/python310.json` | Python 3.10.11 | 93 | | ||
| | `src-tauri/scoop_manifest_templates/dark.json` | WiX Toolset Decompiler (v3.14.1) | 10 | |
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.
The tables in this document (e.g., here, and also at lines 194-197, 220-226, etc.) and various references to workflow files include hardcoded line counts and line ranges. This information is very brittle and will become outdated as soon as the referenced files are modified. This will make the maintenance guide harder to trust and use over time.
It would be more robust to remove the "Lines" column from tables and avoid referencing specific line numbers for code blocks or workflow files. Describing the function or section by name is sufficient and more maintainable.
|
|
||
| - Install ESP-IDF non-interactively: | ||
|
|
||
| `eim install -i {{v5.3.2}}` |
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.
The example for non-interactive installation uses a hardcoded version {{v5.3.2}}. Other examples in this section correctly use a generic placeholder {{version}}. Using a hardcoded version can become outdated and is inconsistent with the rest of the examples.
| `eim install -i {{v5.3.2}}` | |
| `eim install -i {{version}}` |
| | `src-tauri/scoop_manifest_templates/7zip.json` | 7-Zip archiver (v25.01) | 73 | | ||
| | `src-tauri/scoop_manifest_templates/git.json` | Git for Windows (v2.50.1) | 82 | | ||
| | `src-tauri/scoop_manifest_templates/python311.json` | Python 3.11.9 | 95 | | ||
| | `src-tauri/scoop_manifest_templates/python310.json` | Python 3.10.11 | 93 | |
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 table lists a manifest template for Python 3.10 (python310.json). However, reviewing the implementation in src-tauri/src/lib/offline_installer.rs (lines 338-343), it appears that only the python311.json template is actually used for offline installation. The documentation should accurately reflect the code to avoid confusion for maintainers.
Please remove this line for python310.json to align the documentation with the current implementation.
Hahihula
left a 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.
Great work. Thanks.
But do we actually want this to be part of the public facing documentation? I see this more as internal document.
I would suggest moving the md to the project root and removing it from the docs. WDYT?
Added couple of small remarks.
|
|
||
| 3. Scoop then installs each package using the processed manifest. | ||
|
|
||
| ### Version Update Procedure |
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.
important notice:
the real links for where to get. the actually file URLs are currently hardcoded in the offline installed. I do not see any mention of this. this will ikely change during EIM-381 implementation but currently whis update procedure as is will not work!
|
|
||
| --- | ||
|
|
||
| ## 3. Docker Repository |
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.
we have the official docker image in the esp-idf repository which is being migrated to eim currently, probably after next release. we also have the CI images in the esp-dockerfiles and we have dockerfile example in the documentation.
|
|
||
| --- | ||
|
|
||
| ## 11. Scoop Distribution (Online) |
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.
we do not have scoop repository at the moment and the workflow is turned off
docs/src/MAINTENANCE.md
Outdated
|
|
||
| ## Maintenance Checklists | ||
|
|
||
| ### After Each Release |
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.
werify correct version of eim AND offline installer archives is on dl.espressif
docs/src/MAINTENANCE.md
Outdated
| - [ ] Review AWS IAM permissions | ||
| - [ ] Check for security advisories on dependencies | ||
| - [ ] Update base images (Docker, etc.) | ||
|
|
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.
update dependencies? (suggestion)
docs/src/MAINTENANCE.md
Outdated
| - [ ] Update install-esp-idf-action if CLI interface changed | ||
| - [ ] Update documentation references | ||
|
|
||
| ### Quarterly Security Review |
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.
!check signing certificates expiration dates!
docs/src/MAINTENANCE.md
Outdated
|
|
||
| --- | ||
|
|
||
| ## Maintenance Checklists |
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 would suggest to actually move the maintenance checklist to the top of this document and have the (great) detailed description under it as we will use the checklist much more often :-)
|
@Hahihula fixed the review comments kindly take a look |
Description
Created a maintenance guide document
The information needs to be verified.
The workflow files mentioned here were just for my understanding but i have left them there to be reviewed.