-
Notifications
You must be signed in to change notification settings - Fork 212
feat(templates): add GCP best practices guide #100
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: main
Are you sure you want to change the base?
Conversation
commands/conductor/review.toml
Outdated
| 2. **Guides Compliance:** | ||
| - Does it follow `product-guidelines.md`? | ||
| - Does it strictly follow `conductor/code_styleguides/*.md`? | ||
| - Does it adhere to `conductor/platform_guides/*.md` (if applicable)? |
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.
change to (if exists and is applicable) otherwise it might try to search for that file
commands/conductor/newTrack.toml
Outdated
| * Check for the existence of `conductor/platform_guides/google-cloud-platform.md`. | ||
|
|
||
| 2. **Conditional Offer:** | ||
| * **IF** (GCP is mentioned) **AND** (the guide is MISSING): |
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.
would change to GCP keywords are mentioned to ensure the earlier words are included
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.
done
commands/conductor/setup.toml
Outdated
| - Create the directory `conductor/platform_guides/`. | ||
| - Copy `~/.gemini/extensions/conductor/templates/platform_guides/google-cloud-platform.md` to `conductor/platform_guides/google-cloud-platform.md`. | ||
| - Announce: "Added GCP Best Practices guide to `conductor/platform_guides/`." | ||
| - **Commit State:** After handling the guides (Code Style + Platform), you MUST immediately write to `conductor/setup_state.json` with the exact content: |
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.
can we split the style guides and platform guides into two sections? they dont have overlap, and i think here it doesnt make sense to combine them
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.
done
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.
lgtm. are these recommendations from a source?
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.
Responded in the internal chat thread.
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.
Should we append the source like we do for the code style guides? Overall, this documents LGTM.
mahimashanware
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.
left comments
commands/conductor/review.toml
Outdated
| ## Verification Checks | ||
| - [ ] **Plan Compliance**: [Yes/No/Partial] - [Comment] | ||
| - [ ] **Style Compliance**: [Pass/Fail] | ||
| - [ ] **Guides Compliance**: [Pass/Fail] |
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.
same here and in other places, i think style and platform guides should be separate
commands/conductor/review.toml
Outdated
| ## Verification Checks | ||
| - [ ] **Plan Compliance**: [Yes/No/Partial] - [Comment] | ||
| - [ ] **Style Compliance**: [Pass/Fail] | ||
| - [ ] **Guides Compliance**: [Pass/Fail] |
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.
same here and in other places, i think style and platform guides should be separate
| - If `STEP` is "2.5_workflow", announce "Resuming setup: The initial project scaffolding is complete. Next, we will generate the first track." and proceed to **Phase 2 (3.0)**. | ||
| - If `STEP` is "2.4_code_styleguides", announce "Resuming setup: Code Styleguides are configured. Next, we will check for Platform Guides." and proceed to **Section 2.5**. | ||
| - If `STEP` is "2.5_platform_guides", announce "Resuming setup: Platform Guides are checked. Next, we will define the project workflow." and proceed to **Section 2.6**. | ||
| - If `STEP` is "2.6_workflow", announce "Resuming setup: The initial project scaffolding is complete. Next, we will generate the first track." and proceed to **Section 2.7**. |
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.
@mahimashanware @moisgobg ptal at the reference. I will them to be fixed in the separate ticket, but wanted to make sure this one is still correct semantically.
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 think this is better, because by saying "go to 3.0" before, we might've skipped the 2.7 step.
LGTM
Fixes #98.
Ran local tests:
https://screenshot.googleplex.com/rEQXuha8i38tBnE
https://screenshot.googleplex.com/3bRkuzoUMpzRt8Q