Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 SummaryAdds a WalkthroughThe change adds a GNS3 deployment script, configures the ChangesGNS3 CI deployment
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Zuul
participant run.yml
participant gns3-deployment.sh
participant Kayobe
Zuul->>run.yml: Start GNS3 check job
run.yml->>gns3-deployment.sh: Execute with KAYOBE_CONFIG_REF
gns3-deployment.sh->>Kayobe: Bootstrap control host
gns3-deployment.sh->>Kayobe: Configure physical network
Merge Risk: 🟠 High · up to The new GNS3 check cannot reliably test the proposed configuration and may fail before deployment. Correct the repository checkout before merging. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 01ef6ca4-dab9-4bfe-b85c-1fbd2cc336b8
📒 Files selected for processing (5)
etc/kayobe/environments/ci-gns3/.kayobe-environmentetc/kayobe/environments/ci-gns3/gns3-deployment.shzuul.d/jobs.yamlzuul.d/playbooks/gns3-base/run.ymlzuul.d/project.yaml
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: stackhpc/check
- GitHub Check: Ansible 2.20 lint with Python 3.12
- GitHub Check: Ansible 2.19 lint with Python 3.12
🧰 Additional context used
🪛 Shellcheck (0.11.0)
etc/kayobe/environments/ci-gns3/gns3-deployment.sh
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🔇 Additional comments (5)
etc/kayobe/environments/ci-gns3/.kayobe-environment (1)
1-3: LGTM!zuul.d/playbooks/gns3-base/run.yml (1)
1-11: LGTM!zuul.d/jobs.yaml (1)
50-78: LGTM!zuul.d/project.yaml (1)
13-13: LGTM!etc/kayobe/environments/ci-gns3/gns3-deployment.sh (1)
2-2: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove the shebang to line 1.
The blank first line causes ShellCheck SC1128. It also prevents direct execution from selecting Bash, which this script requires for
[[ ... ]].- #!/bin/bashLikely an incorrect or invalid review comment.
Source: Linters/SAST tools
MoteHue
left a comment
MoteHue
left a comment
There was a problem hiding this comment.
Fake approval to let check-review run.
priteau
left a comment
priteau
left a comment
There was a problem hiding this comment.
Approving to allow Zuul to run
| sudo cp "$GNS3_ROLE_PATH/roles/gns3/files/switch1" \ | ||
| "$KAYOBE_CONFIG_PATH/environments/ci-gns3/inventory/host_vars/switch1" |
There was a problem hiding this comment.
Not something that needs changing now, but it feels a bit weird to have an Ansible role generating files within its own directories that need copying out. Perhaps the role can have a configurable destination directory, which we then set to $KAYOBE_CONFIG_PATH/environments/ci-gns3/inventory/host_vars/switch1 here.
There was a problem hiding this comment.
Yeah this is definitely a good idea, I'll get this implemented soon
MoteHue
left a comment
MoteHue
left a comment
There was a problem hiding this comment.
Approving for check-review job
| X8xTonsuPmuN95hVR/Aq71cNIKvRuS74j0Gbi6c7OrP/twkjsacCFzDUR1v9VF3+r3u4Q | ||
| u6t4qeoutF0oaYs4gkC6vV76Qg3sbgBsUNyHaUnjMxvZzgN+2JynqA5nB/2tfE= | ||
| - secret: | ||
| name: aio_vault_password |
There was a problem hiding this comment.
Can we please not use the aio credentials in Zuul? In the past there was a security issue and we had to rotate all the secrets, which is why we separated them in the past
There was a problem hiding this comment.
Will remove now, sorry about that
|
Happy Friday @L-Chams, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
4 participants
No description provided.