Add policies validation workflow#2
Merged
Merged
Conversation
Reviewer's GuideAdds a new validation and release workflow for Updatecli policies, including a Bash script to validate and optionally publish policies, a Python doc generator for policy metadata, a GitHub Actions workflow to run tests and e2e checks, and Makefile targets to orchestrate these operations. Sequence diagram for the new policy validation and release workflowsequenceDiagram
actor Developer
participant GitHubActions
participant Makefile
participant release_bash as release.bash
participant updatecli as UpdatecliCLI
Developer->>GitHubActions: push / pull_request
GitHubActions->>Makefile: make test
Makefile->>release_bash: .ci/scripts/release.bash
loop For each Policy.yaml
release_bash->>release_bash: validateRequiredFile
alt e2e tests on main
GitHubActions->>Makefile: make e2e-test
Makefile->>release_bash: .ci/scripts/release.bash --e2e-test
release_bash->>release_bash: runUpdatecliDiff
release_bash->>updatecli: updatecli diff
end
alt publish
Developer->>Makefile: make release
Makefile->>release_bash: .ci/scripts/release.bash --publish
release_bash->>updatecli: updatecli manifest push
end
end
Flow diagram for the policy documentation generation scriptflowchart TD
A[Start docgen.py] --> B[find_policy_yaml]
B --> C{Policy.yaml files found?}
C -- No --> D[Print 'No Policy.yaml files found.'] --> Z[End]
C -- Yes --> E[For each Policy.yaml: load_policy_metadata]
E --> F[generate_markdown_table]
F --> G[Write POLICIES.md]
G --> Z[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
.ci/scripts/docgen.py, the nested f-string ingenerate_markdown_table({f"[link]({readme_url})" }) is invalid Python syntax; build the link string separately or inline it without a nested f-string, and consider removing the unused top-leveloriginal_url/new_urlexample code which currently runs at import time. - The
Makefilehas.PHONY: validatefor thetestande2e-testtargets, which should instead listtestande2e-test, and thehelprecipe line must start with a tab rather than spaces ormake helpwill fail. - The Bash validation in
.ci/scripts/release.bashrelies onsed/grepagainstPolicy.yamlandCHANGELOG.mdwith hard-coded patterns and line ranges, which is quite brittle; consider using a YAML-aware parser or more robust pattern matching so minor formatting changes don’t break the workflow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.ci/scripts/docgen.py`, the nested f-string in `generate_markdown_table` (`{f"[link]({readme_url})" }`) is invalid Python syntax; build the link string separately or inline it without a nested f-string, and consider removing the unused top-level `original_url`/`new_url` example code which currently runs at import time.
- The `Makefile` has `.PHONY: validate` for the `test` and `e2e-test` targets, which should instead list `test` and `e2e-test`, and the `help` recipe line must start with a tab rather than spaces or `make help` will fail.
- The Bash validation in `.ci/scripts/release.bash` relies on `sed`/`grep` against `Policy.yaml` and `CHANGELOG.md` with hard-coded patterns and line ranges, which is quite brittle; consider using a YAML-aware parser or more robust pattern matching so minor formatting changes don’t break the workflow.
## Individual Comments
### Comment 1
<location path=".ci/scripts/docgen.py" line_range="46" />
<code_context>
+ description = p.description.replace("\n", " ").strip()
+ ghcr_path = f"ghcr.io/{os.path.normpath(os.path.dirname(p.path))}"
+ readme_url = replace_filename_in_url(f"https://github.com/ContainerCraft/updatecli-policies/tree/main/{p.path}", "README.md")
+ rows.append(f"| `{ghcr_path}:{p.version}` | {description or '-'} | {f"[link]({readme_url})" } |")
+ return header + separator + "\n".join(rows)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Nested f-strings here are invalid syntax and will cause the script to fail to run.
You can’t embed an f-string inside another like this. Build the link separately and interpolate it, for example:
```python
link = f"[link]({readme_url})"
rows.append(f"| `{ghcr_path}:{p.version}` | {description or '-'} | {link} |")
```
or inline without nesting:
```python
rows.append(f"| `{ghcr_path}:{p.version}` | {description or '-'} | [link]({readme_url}) |")
```
</issue_to_address>
### Comment 2
<location path="Makefile" line_range="15" />
<code_context>
+
+.PHONY: help
+help: ## Show this Makefile's help
+ @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
</code_context>
<issue_to_address>
**issue (bug_risk):** The help command line is indented with spaces instead of a tab, which will break the Makefile.
As written, this target will likely fail with a `*** missing separator` error. Replace the leading spaces on the recipe line with a single tab to ensure `help` runs correctly.
</issue_to_address>
### Comment 3
<location path=".ci/scripts/release.bash" line_range="134" />
<code_context>
+
+ GLOBAL_ERROR=0
+
+ for POLICY in $POLICIES
+ do
+ echo ""
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unquoted expansion of POLICIES in the loop may misbehave if paths contain whitespace or glob characters.
`for POLICY in $POLICIES` depends on word-splitting and globbing and will break if any path has spaces or wildcard chars. Consider iterating safely, e.g.:
```bash
mapfile -t POLICIES < <(find "$POLICIES_ROOT_DIR" -name 'Policy.yaml')
for POLICY in "${POLICIES[@]}"; do
...
done
```
Suggested implementation:
```shell
GLOBAL_ERROR=0
mapfile -t POLICIES < <(find "$POLICIES_ROOT_DIR" -name 'Policy.yaml')
for POLICY in "${POLICIES[@]}"
do
```
1. Ensure that `POLICIES_ROOT_DIR` is defined before calling `main` (for example, the root directory under which policies live).
2. If `POLICIES` was previously being set elsewhere (e.g., as a space-separated list), remove or adjust that logic so that `mapfile` is the single source of truth for the list of policy files.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| GLOBAL_ERROR=0 | ||
|
|
||
| for POLICY in $POLICIES |
There was a problem hiding this comment.
suggestion (bug_risk): Unquoted expansion of POLICIES in the loop may misbehave if paths contain whitespace or glob characters.
for POLICY in $POLICIES depends on word-splitting and globbing and will break if any path has spaces or wildcard chars. Consider iterating safely, e.g.:
mapfile -t POLICIES < <(find "$POLICIES_ROOT_DIR" -name 'Policy.yaml')
for POLICY in "${POLICIES[@]}"; do
...
doneSuggested implementation:
GLOBAL_ERROR=0
mapfile -t POLICIES < <(find "$POLICIES_ROOT_DIR" -name 'Policy.yaml')
for POLICY in "${POLICIES[@]}"
do
- Ensure that
POLICIES_ROOT_DIRis defined before callingmain(for example, the root directory under which policies live). - If
POLICIESwas previously being set elsewhere (e.g., as a space-separated list), remove or adjust that logic so thatmapfileis the single source of truth for the list of policy files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce automated validation, testing, and release support for Updatecli policies, including CI integration.
New Features:
Enhancements:
CI: