Develop/spapadop#28
Merged
Merged
Conversation
… UID/GID lookups and conditional rename/create logic for the dev account.
…tall to prevent breakage from stale UID/GID ownership
…tup failure on missing directory
…tion and remove conflicting filepath tag so valid absolute paths pass consistently
… shell quoting for user/group and path arguments
…h is used and venv python is missing
… setup to prevent stale-dotfile permission failures (e.g. .bashrc)
…v-startup-script-so-generated-devenvs-boot Spapadop/plt 859 stabilize devenv startup script so generated devenvs boot
…h-validation-incorrectly-uses-filepath Spapadop/plt 881 volumemount path validation incorrectly uses filepath
…sage-in-rendered-manifest-files spapadop/plt-862-fix-namespace-usage-in-rendered-manifest-files
…match StatefulSet serviceName while preserving existing SSH/HTTP services
…vicename-does-not-match-any-generated-service Spapadop/plt 849 statefulset servicename does not match any generated service
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
eywalker
approved these changes
Mar 6, 2026
Contributor
eywalker
left a comment
eywalker
left a comment
There was a problem hiding this comment.
Looks great -- thanks for catching all those subtle bugs
| @@ -1,7 +1,26 @@ | |||
| apiVersion: v1 | |||
| kind: Service | |||
| metadata: | |||
Contributor
There was a problem hiding this comment.
What's this extra SSH port for?
Collaborator
Author
There was a problem hiding this comment.
This was the reply I got when I asked ChatGPT at the time:
Great question. Practical implications are:
- With the port (22) on the headless governing Service:
- Service is clearly valid/standard and reliably accepted by Kubernetes API.
- EndpointSlices include a named port (ssh), so SRV-based discovery can work.
- You get no extra external exposure by itself, because it is still headless (clusterIP: None) and not NodePort/LoadBalancer.
- Without any port on that Service:
- You still get the main StatefulSet DNS identity behavior (pod hostnames) in many setups.
- But Service definitions without ports are less standard and can be rejected depending on API validation/version/config.
- You lose explicit service-port metadata (and SRV records tied to named ports).
- Bottom line:
- The port is mostly low-risk structural correctness and compatibility.
- It does not replace or affect external SSH access; that still comes from devenv-ssh-{{.Name}} NodePort service.
| if id -g ${TARGET_GID} &>/dev/null; then | ||
| echo "Renaming group ${TARGET_GID} to ${DEV_USERNAME}" | ||
| groupmod -n ${DEV_USERNAME} $(id -gn ${TARGET_GID}) | ||
| if GROUP_ENTRY="$(getent group "${TARGET_GID}")"; then |
Contributor
There was a problem hiding this comment.
Curious to learn what was the problem with id -g?
Collaborator
Author
There was a problem hiding this comment.
getent is better here because you’re checking numeric UID/GID existence, not “user by name”.
Why id was wrong in this context:
- id -u ARG / id -g ARG interpret ARG as a username, not a UID/GID lookup key.
- So with TARGET_UID=10000, id -u 10000 means “user literally named 10000”, not “does UID 10000 exist?”
That can send logic down the wrong branch (try useradd/groupadd when entries already exist).
Why getent fits:
-
getent passwd does NSS lookup by UID key.
-
getent group does NSS lookup by GID key.
-
It works across configured identity sources (/etc/passwd, LDAP, etc.) through NSS, same source of truth account tools use.
Practical impact in your script:
- Correctly detects existing UID/GID mappings.
- Makes create/rename logic truly idempotent.
- Avoids false negatives and collision failures from mis-detection.
| else | ||
| echo "Adding user ${DEV_USERNAME} with UID ${TARGET_UID}" | ||
| useradd -u ${TARGET_UID} -m -s /bin/bash ${DEV_USERNAME} | ||
| useradd -u "${TARGET_UID}" -g "${TARGET_GID}" -m -s /bin/bash "${DEV_USERNAME}" |
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
2 participants
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.
Closes PLT-859 #24
Closes PLT-881 #25
Closes PLT-862 #26
Closes PLT-849 #27