Repository navigation
Conversation
…ed text (#1712) ## Summary - Removes every em dash from this repository's public text: 1626 of them across 357 files, in the Markdown docs, the code comments, the workflows, the Debian packaging and two generated surfaces. - Each is rewritten sentence by sentence, not substituted. A spaced hyphen left behind would be the same tell the standard warns about. - No behaviour change, and no line added or removed anywhere. ## Changes - **Markdown, 161 across 9 files.** `docs/commands.md` carried 48, `docs/docker-migration.md` 26, `docs/self-update.md` 23. Three `README.md` headings changed shape (`### Optional: macOS`), which moves their anchors; I grepped the tree and nothing links to them. - **Code comments, roughly 1440.** `internal/compose/types/service.rs` alone had 98, all of one shape: a doc comment naming a backticked compose key, then a dash, then the description. A colon there would read as a doubled colon, since the compose key already ends in one, so those 98 took a verb instead (`` `image:` is the container image reference ``). The booleans do not take a verb, so there the dash became a comma. - **Generated text kept in step with its generator.** `bench/chart.py` writes the SVG title, so `docs/assets/bench.svg` changed with it. `about.hbs` renders the NOTICES attribution line. - **`debian/changelog`, 27**, all in entries for 2.0.0 through 4.0.0. `dpkg-parsechangelog` still reads the file. - **Deliberately left alone:** `bench/aggregate.py` returns an em-dash string for a table cell with no measurement. That is a data glyph rather than punctuation, and changing it would alter published benchmark output without removing any tell. `git diff --numstat` shows every file at equal insertions and deletions, so `line-limit` cannot move and the diff reads one line for one line. ## Test plan - [x] `cargo fmt --all --check` is clean - [x] `cargo clippy --locked --all-targets --all-features -- -D warnings` is clean - [x] `cargo test --locked --all-features --workspace` passes locally (2385 tests, 0 failures) - [x] `./tests/shell/*.test.sh` pass locally (every tracked test, including `ci-runs-every-test.test.sh`) - [x] shellcheck is clean on every tracked `*.sh` and every extensionless script with a shell shebang - [ ] Podman lane ran (live Podman 5 and Podman 6, in the PR's own check run) for changes to `internal/`, the engine integration, or the libpod REST adapter - [ ] Asset-contract fixtures ran for changes to `install.sh`, `install.ps1`, `internal/update/install.rs`, the signing scripts or the release fixtures The last two run in this PR's own checks rather than locally; `internal/` and `install.sh` are both touched, comments only in each. This PR adds and changes no check, so there is no control to delete and watch go red. What it does touch is the wording of existing log and error strings, and I grepped the suites for every reworded string first to confirm none is asserted on. - [ ] New or changed checks were verified by deleting the control and watching them fail ## Checklist - [x] Targets `develop` (the release pull request that targets `main` is the one exception, gated by `reusable-main-guard.yml`) - [x] Commits are signed off (DCO, `git commit -s`) - [x] Commits are signed (`required_signatures` is enforced on `develop` and `main`) - [x] Labels applied (`type:`, `prio:`, `effort:`, `area:` where applicable) - [x] Every new file in `tests/shell/` is named in some workflow's `test-command` (no new file here) - [x] No secrets, keys or credentials in code, logs or fixtures - [x] Docs updated if behaviour changed (behaviour is unchanged; the docs change is the point of the PR) - [x] `Cargo.toml` and `debian/changelog` are at the same version (neither version line is touched) - [x] `cargo build --release --locked --bin podup` fits `bench/binary-budget-mib` for changes that can move the binary size (comments and docs cannot move it) ## Related issues Refs #1711. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
… itself (#1713) ## Summary - `podup audit` no longer reports `unpinned_image` for a service whose `pull_policy` already forbids a fetch (`build` or `never`) while it builds the tag itself. - Where the policy still allows a fetch, the finding stays but names the fix that applies: set `pull_policy: build`. A digest cannot anchor a tag that exists in no registry. - Fixes #1710. ## Changes - `internal/audit/checks.rs`: `check_unpinned_image` now consults `service.build` and `service.pull_policy`. The digest guard, the missing-`image:` guard and the tag parsing are untouched; the new branches sit after them. - `internal/audit/checks_more_tests.rs`: five tests, one per row of the matrix below that the suite did not already cover. Two assert on the message text, because two rows differ from the existing behaviour only in wording. Deliberately **not** a blanket skip on `build.is_some()`. With `build:` and `image:` both present and the policy at its default, Compose pulls when the image is absent, and `pull_services_with_options` (`internal/engine/build/pull.rs`) filters on `image.is_some()` with no `build:` exclusion, so `podup pull` does try to fetch such a tag. A blanket skip would trade this false positive for a blind spot on a real registry image that also carries a build fallback. | `build:` | `image:` | `pull_policy:` | before | after | |---|---|---|---|---| | absent | `foo:latest` | default | flagged | flagged, wording unchanged | | present | absent | default | not flagged | not flagged | | present | `foo:latest` | default | flagged, "moves under you" | flagged, names `pull_policy: build` | | present | `foo:latest` | `build` | flagged | not flagged | | present | `foo:latest` | `never` | flagged | not flagged | | present | `foo:latest` | `always` | flagged, "moves under you" | flagged, names `pull_policy: build` | ## Test plan - [x] `cargo fmt --all --check` is clean - [x] `cargo clippy --locked --all-targets --all-features -- -D warnings` is clean - [x] `cargo test --locked --all-features --workspace` passes locally - [x] `./tests/shell/*.test.sh` pass locally (every tracked test, including `ci-runs-every-test.test.sh`) - [x] shellcheck is clean on every tracked `*.sh` and every extensionless script with a shell shebang - [ ] Podman lane ran (live Podman 5 and Podman 6, in the PR's own check run) for changes to `internal/`, the engine integration, or the libpod REST adapter - [ ] Asset-contract fixtures ran for changes to `install.sh`, `install.ps1`, `internal/update/install.rs`, the signing scripts or the release fixtures The bug was reproduced against the shipped v5.9.1 binary before the change, on both the default policy and `pull_policy: build`, which is how I found that the policy was not being read at all. After it, the built binary reports the new message on the default-policy file and nothing at all on the `pull_policy: build` one. Sabotage: I restored the old `check_unpinned_image` body while keeping the new tests, confirmed with `diff -q` that the file really changed, and ran them. Four of the five went red (the three policy rows and the new-wording row); the fifth stayed green, which is correct, because it pins the behaviour for a service with no `build:` and that behaviour is deliberately unchanged. - [x] New or changed checks were verified by deleting the control and watching them fail ## Checklist - [x] Targets `develop` - [x] Commits are signed off (DCO, `git commit -s`) - [x] Commits are signed - [x] Labels applied - [x] No secrets, keys or credentials in code, logs or fixtures - [x] Docs updated if behaviour changed - [x] `Cargo.toml` and `debian/changelog` are at the same version ## Related issues Refs #1710. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…ngs (#1714) ## Summary - `podup audit` no longer reports `secret_in_environment` for a name that merely *contains* `PASSWORD`, `SECRET`, `TOKEN` or `KEY`. `MONKEY_HABITAT` and `MD_Keywords` are silent; `DB_PASSWORD`, `apiToken` and `API_T_ClientSecret` still fire. - The name is split into segments and each segment is compared for equality, so `KEYWORDS` is not `KEY`. - Fixes #1709. ## Changes - `internal/audit/checks.rs`: new private `segments()` helper splitting on `_`, `-`, `.` and at camelCase/PascalCase boundaries; `check_secret_in_environment` compares segments for equality. The constant is renamed from `SECRET_NAME_SUBSTRINGS` to `SECRET_NAME_SEGMENTS`, because a name that says "substring" next to equality matching is a name that will mislead the next reader. - `internal/audit/checks_more_tests.rs`: six tests. One per group of the issue's truth table, one pinning the exact segment list for all ten rows (so a split regression surfaces before the verdict tests can pass by accident), and one for the non-ASCII case below. - A second commit fixes an unrelated stray `back , the` in a neighbouring comment. It predates the em-dash sweep in `eb29897`; I only noticed it because I was editing the file. No deny list of words like `KEYWORDS` or `KEYRING`. Segment equality gets the same result without a list that never stops growing. | name | segments | before | after | |---|---|---|---| | `MONKEY_HABITAT` | MONKEY, HABITAT | flagged | silent | | `MD_Keywords` | MD, KEYWORDS | flagged | silent | | `MD_Terminos` | MD, TERMINOS | silent | silent | | `API_X_Token` | API, X, TOKEN | flagged | flagged | | `API_T_ClientSecret` | API, T, CLIENT, SECRET | flagged | flagged | | `API_YT_Key` | API, YT, KEY | flagged | flagged | | `DB_PASSWORD` | DB, PASSWORD | flagged | flagged | | `AWS_SECRET_ACCESS_KEY` | AWS, SECRET, ACCESS, KEY | flagged | flagged | | `apiToken` | API, TOKEN | flagged | flagged | | `my.secret.value` | MY, SECRET, VALUE | flagged | flagged | ### One case the issue did not cover A non-ASCII byte counts as the lower side of a camelCase boundary. The first version of the split required `prev.is_ascii_lowercase()`, and the lead byte of an n-tilde is not, so a key spelled `<n-tilde>Key` was swallowed whole and went silent, when `contains` had reported it. That is a false negative introduced while removing a false positive, which is a worse trade than the bug. One extra term and a test pin it. Measured against the built binary: `SENA_SECRET` (with n-tilde) and `<n-tilde>Key` fire, `CLAVE_MANANA` (with n-tilde) does not. ### The test file is now warning on line-limit Rebased onto `develop` after #1713 landed. `internal/audit/checks_more_tests.rs` is at 432 code lines: 242 before either audit fix, 342 after #1713, 432 after this one. The soft limit is 300 and the hard limit is 500, so the gate warns and does not fail. I only caught this after the rebase, because on #1713 I checked `wc -l` against 500 rather than the code-line count against 300, which is the number the gate reads. Tracked as #1715 rather than folded in here: splitting the file is a separate change and would bury the fix. ## Test plan - [x] `cargo fmt --all --check` is clean - [x] `cargo clippy --locked --all-targets --all-features -- -D warnings` is clean - [x] `cargo test --locked --all-features --workspace` passes locally (33 test binaries, 0 failures) - [x] `./tests/shell/*.test.sh` pass locally - [x] shellcheck is clean on every tracked `*.sh` - [ ] Podman lane ran (live Podman 5 and Podman 6, in the PR's own check run) - [ ] Asset-contract fixtures ran Reproduced against the shipped v5.9.1 binary first: `MD_Keywords` and `MONKEY_HABITAT` reported, `MD_Terminos` (same value, different name) not, which isolates the name match as the cause. `audit --strict` exited 1. The built binary reports neither afterwards. Sabotage: reverting `checks.rs` wholesale is not a valid sabotage here, because the segment-contract test calls the helper and the build fails, which is red for the wrong reason. So I put the `contains` call back and left the helper in place, which reintroduces the defect and nothing else. Two tests went red, `silent_when_keyword_only_appears_as_substring` and `silent_when_segment_only_starts_with_keyword`; the other eight stayed green, which is correct, because they pin behaviour `contains` also satisfied. - [x] New or changed checks were verified by deleting the control and watching them fail ## Checklist - [x] Targets `develop` - [x] Commits are signed off (DCO, `git commit -s`) - [x] Commits are signed - [x] Labels applied - [x] No secrets, keys or credentials in code, logs or fixtures - [x] Docs updated if behaviour changed - [x] `Cargo.toml` and `debian/changelog` are at the same version ## Related issues Refs #1709. --------- Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
…test files (#1716) ## Summary - Splits `internal/audit/checks_more_tests.rs`, which had reached 432 code lines against a soft limit of 300, into three files by check. - Pure move. No test renamed, merged, deleted or reworded. - Fixes #1715. ## Changes - New `internal/audit/checks_secret_env_tests.rs` and `internal/audit/checks_unpinned_image_tests.rs`, registered in `checks.rs` alongside the two module declarations already there. - `checks_more_tests.rs` keeps the checks that did not move, with its `//!` header corrected so it no longer describes contents that left. - `checks_tests.rs` untouched. The two checks that moved also had their tests scattered: `secret_in_environment` sat in two separate blocks and `unpinned_image` in two more, because the tests added with #1713 and #1714 were appended at the end rather than beside their siblings. Each check now reads top to bottom in one place. ## Test plan - [x] `cargo fmt --all --check` is clean - [x] `cargo clippy --locked --all-targets --all-features -- -D warnings` is clean - [x] `cargo test --locked --all-features --workspace` passes locally - [x] `./tests/shell/*.test.sh` pass locally - [x] shellcheck is clean on every tracked `*.sh` - [ ] Podman lane ran (live Podman 5 and Podman 6, in the PR's own check run) - [ ] Asset-contract fixtures ran **The control for a move is the test-name list, not the suite.** A refactor that quietly drops a test goes green exactly like one that does not. So the invariant checked here is that the set of bare test function names is identical before and after, compared with: ``` cargo test --locked --all-features --bin podup -- --list \ | grep '^audit::' | sed 's/.*:://' | sed 's/: test$//' | sort ``` Fully qualified names do change, since the module path moves; the bare names must not. This PR adds no check, so there is no control to delete and watch fail. The move itself is verified by the list above rather than by a new assertion. - [x] New or changed checks were verified by deleting the control and watching them fail ## Checklist - [x] Targets `develop` - [x] Commits are signed off (DCO, `git commit -s`) - [x] Commits are signed - [x] Labels applied - [x] No secrets, keys or credentials in code, logs or fixtures - [x] Docs updated if behaviour changed (no behaviour change) - [x] `Cargo.toml` and `debian/changelog` are at the same version ## Related issues Refs #1715. Signed-off-by: Jaro-c <75870284+Jaro-c@users.noreply.github.com>
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.
Merge commit, no tag. Four commits past 5.9.1:
auditreportingunpinned_imageon a tag the service builds itself.secret_in_environmenton name segments instead of substrings.Unlike the last promotion this one is not documentation only: #1713 and #1714 change behaviour in
internal/audit/checks.rs, so main ends up carrying two audit fixes that no published binary has. I am still promoting without a tag, because the version is untouched on both sides (Cargo.tomlanddebian/changelogboth read 5.9.1) and a tag would have to disagree with them. Whether those two fixes are worth a 5.9.2 is a separate call.Signed-off-by: Jaro-c 75870284+Jaro-c@users.noreply.github.com