fix: stop two false warnings on every deploy, plus two papercuts - #13
Merged
Conversation
Four things surfaced while testing PVC behaviour on a live cluster. None change
a single byte of generated YAML — verified by diffing the full render before and
after.
**A named volume warned about a file that cannot exist.** ParseVols builds
MountPath as "<host>:<container>", and a named volume has no host, so it came
out as ":/var/lib/postgresql/data". That string was then handed to os.Stat to
decide whether it was a config file:
level=warning msg="File don't exist or failed to check if the directory is
empty: stat :/var/lib/postgresql/data: no such file or directory"
Once per volume, on every deploy, and nothing a user can do about it. Only a
bind mount has a host path worth inspecting, so the check is now guarded on
one. Both return values were false for named volumes anyway, so the outcome is
unchanged — including the fact that the loop overwrites the value the
kompose.volume.type label sets, which is left alone here rather than fixed
blind.
**`external: true` claimed it was ignored while honouring it.** The fork warned
"External secrets X is not currently supported - ignoring" and then wired the
volume to that exact Secret, correctly. The message was the only thing wrong;
it now says what happens, at info level, because nothing is wrong.
**`secret get` header ignored its own tabwriter**, printing raw tabs so the
header landed on terminal tab stops while the table below it was aligned.
**Two live e2e tests shared the container name "orcinus-pl"**, and liveCluster
does `docker rm -f <name>` on the way in — so whichever ran second tore down
the other's cluster. Only latent while they run sequentially.
TestConvertNamedVolumeIsQuiet captures logrus output and fails if the stat
warning returns; confirmed it fails without the fix. The existing
TestConvertBindMounts already covers the behaviour the guard could have broken
(named volume → PVC, bind mount → hostPath, readonly preserved).
Reviewing the message this branch introduced turned up that its `else` covers
three different situations, and the new wording is only right for one of them.
- `external: true` — the Secret exists in the cluster. Info, as before.
- `environment: VAR` — compose would read the value from that variable; orcinus
does not, so no Secret is created and a service referencing it points at
something that will never exist. That has to warn, naming the variable.
- anything else — no `file:`, not external. Warns plainly.
Saying "using the one in the cluster" for the middle case would have been worse
than the original message, since it asserts something that is not true.
The test asserts the level of the line about the secret rather than searching
the whole capture, which unrelated warnings ("won't be created because 'ports'
is not specified") would otherwise satisfy. It also matches on plain phrases:
logrus escapes the quotes inside msg, so looking for `Secret "s"` silently
never fires — the first draft of the quiet-conversion assertion passed for that
reason rather than because it was true.
anak10thn
force-pushed
the
fix/deploy-warnings
branch
from
August 6, 2026 05:45
b9e3d2c to
b755bd3
Compare
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
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.
Four bugs surfaced while testing PVC behaviour on a live cluster. No generated YAML changes — verified by diffing the full render before and after.
1. Every named volume warned about a file that cannot exist
ParseVolsbuildsMountPathas"<host>:<container>". A named volume has no host, so it comes out as":/var/lib/postgresql/data"— and that string was handed toos.Statto decide whether it was a config file:Once per volume, on every deploy, alarming, untrue, and nothing a user can act on.
Only a bind mount has a host path worth inspecting, so the check is now guarded on one. Both return values were
falsefor named volumes anyway, so the outcome is unchanged.2.
external: truesaid it was ignored while honouring itThe fork printed that and then wired the volume to that exact Secret, correctly — as verified in the
x-orcinus-env-from-secretwork. The message was the only thing wrong. Now:Info, not warning, because nothing is wrong. (No
SetLevelanywhere, so logrus's default keeps this visible.)3.
secret getheader ignored its own tabwriterRaw tabs, so the header landed on terminal tab stops while the table below it was aligned. My own papercut from #11.
4. Two live e2e tests shared a container name
TestLivePluginsandTestLivePlacementboth usedorcinus-pl, andliveClusterdoesdocker rm -f <name>on the way in — so whichever ran second tore down the other's cluster. Only latent while they run sequentially, which is why it never bit.Verification
Rendered a compose with a named volume, a bind mount, a read-only bind mount and an external secret, on both binaries:
TestConvertNamedVolumeIsQuietcaptures logrus output and fails if the stat warning returns — confirmed it fails without the fix, so it is not a false pass. The existingTestConvertBindMountsalready covers what the guard could have broken (named volume → PVC, bind mount → hostPath, readonly preserved).148 tests pass here; 202 in the kompose fork's own module.
Not fixed, on purpose
$(date +%s)in acommand:renders as$()(date +%s), which crashes the container with a baresh: syntax errorthat points nowhere near the cause. That is correct Compose behaviour —$must be written$$— so changing interpolation would break the spec. The error surface is genuinely bad though, and a lint that flags$(incommand:would be worth discussing separately.