-
Couldn't load subscription status.
- Fork 6
feat: Editable debug mode #1212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
38587e9 to
6cfe9ea
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1212 +/- ##
=======================================
Coverage 94.64% 94.64%
=======================================
Files 41 41
Lines 2595 2595
=======================================
Hits 2456 2456
Misses 139 139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6cfe9ea to
e451183
Compare
6d39380 to
19bef2f
Compare
| {{- if .Values.debug.enabled }} | ||
| - name: {{ include "blueapi.fullname" . }}-develop | ||
| persistentVolumeClaim: | ||
| claimName: {{ include "blueapi.fullname" . }}-develop | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug.enabled is doing a lot of things on a single boolean- it changes the container that is being used to one that does not start the service by default (allows debugging service startup); it starts a sidecar and mounts config (to allow VSCode to attach); and it would now mount an additional PVC and initContainer in addition to one that may already be configured for scratch purposes, overwriting code changes, so long as the container follows a convention for placement and use of submodule dependencies that is not documented.
We must better decompose those behaviours, as well as documenting the requirements.
I propose we keep the existing scratch configuration/PVC for the copying of code from the container, but make it more in line with what this change is suggesting, while documenting the requirements for mutability:
For the repository plan-bluesky
- pyproject.toml depends on
blueapi,dodaldirectly - Dockerfile places the source for
plan-blueskyanddodalin a predictable location/workspaces/$REPO - Dockerfile may also place the source for other dependencies in
/workspaces/if they may need to be editable
For the blueapi repository:
- Dockerfile places the source for
blueapiin a predictable location/workspaces/$REPO Dockerfilehas a-debugimage tag that does not start the service by default to allow debugging container startup by execing into the pod
For both:
- Dockerfile runs the
blueapiCLI - Helm chart from blueapi repository can be used,
-debugimage can be specified in theimage:field - Helm chart reads
scratchconfiguration fromvalues.yamland attempts to copy source from the container, falling back to fetching the latest release (later: allow configuration of specific versions), copying either to scratch on/dls_sw/or a volume as currently configured (now: add field to switch ephemeral/persisted for version/persisted constantly) - When using a PVC for the scratch area, it is
chmod -R o+wrX'd, and the sidecar/mounting to allow fedids to be picked up is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting we get rid of the ability to include arbitrary external repos in the scratch area (i.e. no more git cloning at all)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
falling back to fetching
the latest release (later: allow configuration of specific versions)
falling back to fetching from the default branch of the remote_url from the ScratchRepository config (later: allow configuration of specific refs)*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And with this current setup, the pvc is deleted when you disable debug mode, are you suggesting getting rid of that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting in add field to switch ephemeral/persisted for version/persisted constantly that that becomes configurable behaviour.
initContainer:
persistentVolume:
enabled: false
# New field, current behaviour is Version, removed when initContainer is disabled is Session?
lifetime: Version | Persistent | Session| - mountPath: /home | ||
| name: home | ||
| - mountPath: /var/run/nslcd | ||
| name: nslcd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these, and /venv would cause issues if debug and initContainer.persistentVolume were enabled simultaneously, as duplicating the entry?
Introduce an editable version of debug mode modelled on fastcs: https://github.com/DiamondLightSource/fastcs
Now setting
debug.enabled: truewill create a new PVC with the source code in it, but settingdebug.enabled: falsewill make that PVC go away and leave you with a stable, known container.Thanks @gilesknap for all your help.