Kubernetes, schedulers, service meshes, container tooling, workflow and job engines.
230 instructions from 9 repositories. Last updated 2026-05-10.
When code depends on ordering/selection (scheduling, routing, matching), implement it using semantic state and explicit guard windows—avoid brittle string checks and avoid guards that assume “typical” transition times.
Apply this standard:
self.hour == set(range(24))) rather than self._orig_hour == '*'.last_run_at.date() == now.date() can block correctness; add tests and prefer guards that won’t fail at boundary transitions.Example (DST guard—avoid brittle hour string checks):
last_offset = last_run_at.utcoffset()
now_offset = now.utcoffset()
is_hourly = (self.hour == set(range(24))) # semantic, parsed check
if (last_offset and now_offset and last_offset > now_offset and is_hourly):
last_utc = last_run_at - last_offset
now_utc = now - now_offset
utc_delta = now_utc - last_utc
# Keep the transition-aware UTC proximity window
if timedelta(0) < utc_delta <= timedelta(hours=2):
# Also verify via tests for midnight/rare transition boundaries
...
Teams should enforce this with targeted tests for DST-at-midnight scenarios and for “logically equivalent” crontab inputs (e.g., '*' vs */1).
When modifying CI workflows, keep pipelines deterministic, resource-efficient, and transparent:
needs: unit-tests).conftest.py) via unexpected directory nesting. Fail fast with clear error messages.Example pattern (job gating):
jobs:
unit-tests:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- run: pytest -m unit
integration-tests:
needs: unit-tests
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- run: pytest -m integration
Example pattern (deterministic discovery):
nested=$(find t/integration -mindepth 2 -name 'test_*.py')
if [ -n "$nested" ]; then
echo "::error::Integration test files must live at t/integration/ root, not in subdirectories: $nested"
exit 1
fi
When adding tests for new behavior, deprecations, or environment-dependent code paths, ensure the test actually exercises the intended branch (avoid vacuous passing) and assert the concrete observable effect.
Apply this checklist: 1) Force the guard conditions
from unittest.mock import MagicMock, patch
with patch.dict(
sys.modules,
{
'gevent': MagicMock(),
# add 'eventlet' similarly if needed
},
clear=False,
):
worker.on_start()
# assert warning/log behavior here
2) Test the user-visible outcome (not just “no crash”)
3) Choose the right level to avoid flakiness
4) Assert precisely
Following these rules prevents tests that “pass vacuously,” improves regression protection for behavior changes/deprecations, and keeps higher-level tests from becoming brittle.
When handling optional/nullable inputs, treat “missing” and “explicitly set to None” as different semantic states.
Practical rules:
None, use a default that keeps the value non-null.
task_id_generator = conf.get("task_id_generator", uuid)
if task_id is None:
task_id = task_id_generator()
time_limit=None (meaning “explicit”), do not collapse it with “argument omitted”. Resolve defaults only when the caller did not provide a value.getattr(x, 'field', None) can be misleading; use presence checks (e.g., __dict__) before applying fallback logic.
actual = getattr(req, 'ignore_result', None)
if isinstance(req, Context) and 'ignore_result' not in req.__dict__:
actual = None # treat as “unset” to allow task-level fallback
if producer is None: return.None and initialize inside the function).When adding or modifying logging:
1) Match logging intent to behavior (level + visibility)
warning)—don’t silently continue.2) Avoid brittle or expensive caller-location tricks
stacklevel or heavy inspect.stack() to make warnings point to the “right” caller—especially when call paths can be indirect.3) Keep log output formatting deterministic
strip(), removing newlines) because it can change observed output and backward compatibility.4) Use the right channel depending on logging configuration availability
sys.stderr is acceptable—goal is visibility, not configuration purity.Example (recommended patterns):
import sys
import logging
logger = logging.getLogger(__name__)
# 1) User-visible fallback
try:
task_id = str(task_id_generator())
except Exception as exc:
logger.warning(
"Custom task_id_generator failed, falling back to UUID: %s: %s",
type(exc).__name__, exc,
)
# 2) Fatal path: critical log + clean shutdown
logger.critical("Retrying to establish connection after connection loss is disabled; shutting down")
raise SystemExit(1)
# 4) Early/deployment message when logging may be unavailable
print("Broken pidfile found", file=sys.stderr)
# 3) Line-buffered proxy write (avoid fragmented output)
class LoggingProxy:
def __init__(self, logger, loglevel=logging.INFO):
self.logger = logger
self.loglevel = loglevel
self._buffer = ""
self.closed = False
def write(self, data: str):
if not data or self.closed:
return 0
self._buffer += str(data)
while "\n" in self._buffer:
line, self._buffer = self._buffer.split("\n", 1)
if line:
self.logger.log(self.loglevel, line)
return len(data)
Adopting these rules prevents misleading or noisy logs, keeps console/traceback output intact, and avoids performance/accuracy pitfalls in warning/caller handling.
Prefer eliminating work that can’t affect observable behavior and prevent resource retention in hot paths.
Apply this as a checklist: 1) Guard mode-dependent logic: if the subsystem doesn’t propagate updates (or the surrounding condition is false), skip the computation/action entirely.
if worker_enable_prefetch_count_reduction:
# If per-consumer QoS (apply_global=False), broker qos updates won’t reach running consumers.
if qos_global is False:
logger.info("Skipping prefetch reduction; per-consumer QoS in effect")
else:
active_count = len(active_requests) # snapshot once
initial_prefetch_count = max(
prefetch_multiplier,
max_prefetch_count - active_count * prefetch_multiplier,
)
2) Run expensive checks/copies only when needed: move detect_* calls and copy.copy(...) inside the “schedule changed” condition.
3) Avoid redundant computation drift: snapshot once and reuse (e.g., active_count = len(active_requests)) rather than recomputing in multiple expressions/logs.
4) Use the right data structures for de-duplication: prefer sets for membership/de-dup and convert at the end, avoiding repeated O(n) in list checks.
5) Prevent memory retention in exception/error handling: clear traceback frame references (e.g., traceback_clear(exc)) and delete local traceback variables (del tb) when they are no longer needed.
6) Avoid “harmless” side-effect calls that allocate resources: in pub/sub flows, don’t ping when there are no subscribers or when ping triggers empty replies/extra allocations.
These changes improve throughput and memory usage without altering functional behavior.
When changing Celery task dispatch semantics (especially Canvas: chains/groups/chords, unroll/freeze, and composed workflows), add regression tests that:
task_id_generator used consistently throughout a canvas workflow).apply_async and confirm only intended tasks/bodies are applied).replaced).Example pattern for asserting the executed path (empty-chain chord header):
# chord whose header contains only empty chains should not apply header tasks
from celery import chord, group, chain
# header member is effectively empty
empty_chain_sig = chain(tuple())
child_count = 24
child_chord = chord([empty_chain_sig], body=add.si(0, 0))
header = group([child_chord] * child_count)
# patch Signature.apply_async and assert only chord bodies run
header.apply_async()
assert mock_apply_async.call_count == child_count
Apply this same approach when testing task-id generation and metadata stamping: freeze signatures, run the canvas workflow, and assert the expected task_id source and _get_task_meta() contents for each composed element.
When a doc section introduces a new configuration option or feature, include accurate version metadata (Celery and, when relevant, the dependency/library version). Missing or incorrect .. versionadded:: causes confusion about availability.
Apply this when you add:
.. setting:: <name> blockTemplate (reST):
.. setting:: my_new_setting
``my_new_setting``
~~~~~~~~~~~~~~~~~~
Default: <value>
.. versionadded:: 5.7
<one- to two-sentence description of what it does>
If the behavior depends on Kombu, specify the Kombu version in the same style (e.g., “.. versionadded:: Kombu 5.6.0”). Avoid adding redundant anchors when using Sphinx directives like .. setting:: that already create anchors.
User-facing configuration should be correct, self-contained, and hard to misuse.
Apply these rules: 1) Document the exact scope of each setting/env var: the description must match the code paths that actually enforce the behavior (what calls it affects, and what it does not affect). 2) Ensure documented config keys are exact: env var/setting names in docs must match the implementation; don’t introduce near-miss names. 3) Avoid “extra required flags” for core behavior: if the system can reliably detect a condition (e.g., quorum queues in use), make the dependent feature enable itself automatically. Treat user warnings as a last resort, not as the primary mechanism. 4) Keep configuration/distribution declarations consistent: if you maintain a canonical requirements list, ensure packaging (e.g., RPM) doesn’t diverge into duplicated or platform-specific dependency sets.
Example (automatic dependent configuration with quorum queues):
from kombu import Queue
task_queues = [Queue('my-queue', queue_arguments={'x-queue-type': 'quorum'})]
# Configure only the primary queue behavior; the dependent feature
# (e.g., native delayed delivery for ETA/Countdown) should enable automatically
# when quorum queues are detected.
broker_transport_options = {"confirm_publish": True}
Checklist when updating configuration/docs:
When handling errors, ensure (1) you don’t accidentally change which exception path fires, (2) recovery/cleanup is deterministic, and (3) any wrapping/aggregation preserves context and remains actionable.
Practical rules:
Example (retryable vs aggregated failures + chaining):
exceptions = []
for item in items:
try:
do_bind(item)
except Exception as e:
if isinstance(e, RETRIED_EXCEPTIONS):
raise # preserve intended retry/outer mechanism
exceptions.append(e)
if exceptions:
raise RuntimeError(
"Binding failures occurred\n" + "\n".join(map(str, exceptions))
)
# During failure-path cleanup:
try:
hub.reset()
except Exception:
pass
try:
hub.timer.clear() # must still run
except Exception:
pass
Apply this standard whenever you touch error recovery, retry/fallback behavior, exception aggregation/wrapping, or shutdown/reconnect paths.
Update documentation every time code behavior changes or new public APIs are introduced.
Apply this checklist:
1) Docstring accuracy: If you change semantics (error handling, return values, shutdown behavior, etc.), update the docstring to match reality.
2) Public API versioning: For any new/changed public method, add the appropriate Sphinx directive (.. versionadded:: X.Y.Z / .. versionchanged:: X.Y.Z) consistently (including any related backend/base/async docstrings).
3) User-facing docs: If behavior impacts users (e.g., shutdown semantics, retries, connection handling), update the relevant documentation section and include a version note.
4) Interface/contract documentation: For new classes/steps that implement an interface, add inline documentation describing the contract/expected behavior.
5) Docs tooling for optional deps: Prefer Sphinx mechanisms like autodoc_mock_imports for optional third-party dependencies so API docs build without hiding misconfiguration.
Example (public API versioning + docstring alignment):
class BaseBackend:
def exists(self, task_id: str) -> bool:
"""Return True if a result exists for the given task id."""
# ...
.. versionadded:: 5.7.0
Example (doc update for shutdown semantics):
.. versionchanged:: X.Y.When changing concurrency/background execution, ensure (a) state is isolated per thread/consumer/instance, and (b) shutdown/cleanup and exception signaling are coordinated with explicit synchronization.
Standards 1) Isolate per-thread/per-consumer resources
threading.local() (or thread-specific state) as a module/class attribute; prefer per-Celery/backend instance or initialize thread-local fields during start().2) Coordinate shutdown with Events and correct lifecycle ownership
threading.Event()/similar primitives to signal stop/shutdown.3) Make exception propagation race-safe
finally, then read the exception after the event).4) Always close/reconnect thread-bound resources
Example pattern
import threading
class BackgroundWorker:
def __init__(self):
self._stop = threading.Event()
self._shutdown = threading.Event()
self._exc = None
self._thread_local = threading.local() # instance-scoped
def start(self):
if getattr(self._thread_local, "started", False):
return
self._thread_local.started = True
self._thread = threading.Thread(target=self._run, daemon=True)
self._thread.start()
def _run(self):
try:
while not self._stop.is_set():
# do work...
pass
except Exception as e:
self._exc = e
finally:
# single point to publish shutdown state
self._shutdown.set()
# close thread-bound resources here
def stop(self):
self._stop.set()
self._shutdown.wait(timeout=10)
if self._exc is not None:
raise self._exc
Code-review checklist
threading.local()? (Fix to per-instance/per-thread.)Events, and is shared state read only after synchronization?Prioritize readability through consistent formatting and modern, simple idioms. For code style changes, ensure multiline constructs are formatted cleanly, complex conditions are made understandable (via extracted “effective” variables or small helpers), repeated patterns are factored out, and modern Python conventions are used.
Apply these practices:
strip() on multiline literals.{} for dict literals, {**d1, **d2} for dict merges, and keep imports ordered (isort).Example (multiline constant + readable conditional):
MAP_ROUTES_MUST_BE_A_DICTIONORY = (
"Starting from Celery 5.1 the task_routes configuration must be a dictionary. "
"Support for providing a list of router objects will be removed in 6.0."
)
effective_exchange = exchange or queue_exchange_name
effective_rkey = routing_key or queue_exchange_routing_key
if not effective_exchange and not effective_rkey and exchange_type == "direct":
# ...
pass
When unsure, prefer the version that is easiest to scan in a code review: small helpers/variables, clear indentation, and minimal unnecessary transformations.
When adding or using caching (e.g., @cached_property, sys.modules-dependent behavior, or internal resolution caches), ensure the cached value depends only on inputs that are already stable.
Standard rules: 1) Don’t cache too early
2) Make runtime-dependent caches test-safe
Example (test isolation for import simulation):
import builtins
from unittest.mock import patch
def failing_import(name, *args, **kwargs):
if name.startswith('gevent'):
raise ImportError('Simulated gevent import failure')
return real_import(name, *args, **kwargs)
real_import = builtins.__import__
with patch.dict('sys.modules', {'gevent': None, 'gevent.monkey': None}):
with patch('builtins.__import__', side_effect=failing_import):
result = is_gevent_monkey_patched()
assert result is False
If you can’t guarantee stable inputs or reliable test isolation, prefer non-cached access (or provide explicit cache invalidation hooks) rather than relying on implicit caching side effects.
Use naming conventions that are both tooling-correct and behavior-semantic:
is_* has side effects, it violates reader expectations. Either rename (e.g., maybe_unlock) or refactor to keep is_* side-effect-free.greenlet over gt).Example patterns to follow:
# Tooling-sensitive: matches `python_classes = "test_*"`
class test_pool_acquire_timeout:
...
# Semantic clarity: numeric timeout vs class
def apply_target(..., timeout=None, TimeoutClass=None, ...):
...
# Misleading name fix: keep `is_*` side-effect free
def is_locked(self):
return os.path.exists(self.path)
def maybe_unlock(self):
if self._is_stale():
self.remove_if_stale()
If you’re unsure, pick the name that answers “what is it?” and “what does it do?” without consulting the implementation.
In configuration/requirements files, make compatibility explicit: bound versions to prevent silent breakage across Python/platform build environments, and use environment markers for dependencies that differ by Python version, OS, implementation, or wheel availability.
Do:
package>=x.y,<x.(y+1) or ~=x.y when appropriate.python_version, sys_platform, or platform_python_implementation.setuptools<82.0.0 when source builds fail with newer setuptools).==) only when you must avoid known incompatibilities or missing wheels; otherwise prefer ranges.Don’t:
<= to == without a clear motivation (it’s a silent change for users depending on transitive compatibility).Example pattern:
# Tooling broken beyond a known version for certain builds
setuptools<82.0.0
# Different tooling versions per Python
pre-commit>=3.5.0,<3.8.0; python_version < '3.9'
pre-commit>=3.8.0; python_version >= '3.9'
# Platform/implementation gating for deps without universal wheels/support
pycurl>=7.43.0.5; sys_platform != 'win32' and platform_python_implementation=="CPython"
# Major-version safety constraints for runtime libs
cassandra-driver>=3.24,<4
When building/installing Python dependencies in Docker/CI for multiple Python versions, keep the environment deterministic and aligned across interpreters: use the same dependency constraint/lock inputs everywhere and ensure the packaging toolchain is consistent per interpreter.
Apply this by:
pip, setuptools, and wheel using the target interpreter.ENV KEY=VALUE style) to avoid lint/tooling drift.Example (adapted):
RUN pyenv exec python3.11 -m pip install --upgrade pip setuptools wheel \
&& --mount=type=cache,target=/home/$CELERY_USER/.cache/pip \
pyenv exec python3.11 -m pip install -r requirements/test.txt \
--build-constraint requirements/constraints.txt
# Prefer consistent ENV syntax
ENV PYTHONUNBUFFERED=1
ENV PYTHONIOENCODING=UTF-8
When building Docker images, optimize dependency installation for both caching and resource limits.
Apply these rules:
1) Maximize layer caching: install Python requirements before copying your application source.
2) Cache package downloads: use BuildKit cache mounts for pip to avoid re-downloading on rebuilds.
3) Bound resource usage: split large multi-version dependency installs into separate RUN steps (avoid one massive chained install) to prevent ResourceExhausted and reduce memory pressure.
Example pattern (mimics the discussed approach):
# Define pyenv versions
RUN pyenv local 3.13 3.12 3.11 3.10 3.9 3.8 pypy3.10
# Install requirements first for layer caching
RUN --mount=type=cache,target=/home/$CELERY_USER/.cache/pip \
pyenv exec python3.13 -m pip install -r requirements/default.txt \
-r requirements/dev.txt -r requirements/docs.txt
RUN --mount=type=cache,target=/home/$CELERY_USER/.cache/pip \
pyenv exec python3.12 -m pip install -r requirements/default.txt \
-r requirements/dev.txt -r requirements/docs.txt
# Copy app after deps
COPY --chown=1000:1000 . $HOME/celery
# Install app last (optionally with --no-deps since deps are already installed)
RUN --mount=type=cache,target=/home/$CELERY_USER/.cache/pip \
pyenv exec python3.13 -m pip install --no-deps -e $HOME/celery
If a single combined install step causes ResourceExhausted, keep (or further refine) the split into smaller RUN layers. Ensure toolchain setup used by dependency builds is deterministic and architecture-aware to avoid rebuild failures across environments.
When changing code that affects a public interface (method signatures, CLI command arguments, plugin discovery, type/subscriptable behavior), treat it as an API contract: keep existing call patterns working, and ensure runtime semantics match the intended contract.
Practical rules:
__class_getitem__ / subscripting), ensure the runtime object you return matches the expected semantics (e.g., types.GenericAlias) and add a small runtime test.Example (signature stability):
class EagerResult:
# Keep legacy parameters in the signature even if the internal behavior no longer needs them.
def get(self, timeout=None, propagate=True, disable_sync_subtasks=True, **kwargs):
if self.successful():
return self.result
if self.state in states.PROPAGATE_STATES:
if propagate:
raise self.result if isinstance(self.result, Exception) else Exception(self.result)
return self.result
Example (CLI input contract parsing):
def revoke_by_stamped_header(state, header, terminate=False, signal=None, **kwargs):
# Support either a list of headers or a list containing header=value pairs.
headers = header if isinstance(header, list) else [header]
parsed = []
for h in headers:
if '=' in h:
k, v = h.split('=', 1)
parsed.append((k, v))
else:
parsed.append(h)
# ... use `parsed` with clear semantics
Add/adjust unit tests to lock in the contract (signature compatibility, parsing shapes, and runtime return types).
Any data placed into connection URIs or transmitted over the network (e.g., JSON payloads/headers) must be encoded/serialized according to the protocol’s rules.
A-Z a-z 0-9 - . _ ~) and “sub-delims” (! $ & ' ( ) * + , ; =).UUID objects—convert to a string representation (commonly .hex or str(uuid)).Example:
from uuid import uuid4
from urllib.parse import quote
user = quote('alice+team', safe='')
password = quote('p@ss:word', safe='')
uri = f"amqp://{user}:{password}@host/vhost"
# For JSON/headers:
monitoring_id = uuid4().hex # not uuid4() directly
headers = {"monitoring_id": monitoring_id}
Choose efficient algorithms and data structures to minimize computational overhead and improve code maintainability. Prefer positive logic over elimination methods, reuse existing data structures instead of creating new ones, and extract common algorithmic patterns into shared functions.
Key principles:
Use positive validation logic: Instead of enumerating all failure cases, validate for the expected success condition. For example, check if a node status contains only “Ready” rather than checking for “NotReady” or “Unknown”.
Reuse existing data structures: Before adding new fields, evaluate if existing structures can be modified or reused. This reduces memory overhead and complexity.
Extract common algorithmic patterns: When similar logic appears in multiple places, extract it into shared functions to improve maintainability and reduce duplication.
Choose efficient comparison methods: Use built-in efficient methods like reflect.DeepEqual() for complex comparisons instead of manual field-by-field checks.
Optimize string operations: Use strings.Builder for concatenating multiple strings instead of repeated string concatenation, which has O(n²) complexity.
Example of efficient validation logic:
// Inefficient: elimination method requiring enumeration
if slices.Contains(status, "NotReady") || slices.Contains(status, "Unknown") {
return false
}
// Efficient: positive validation
if len(status) == 1 && slices.Contains(status, string(v1.NodeReady)) {
return true
}
Example of efficient string building:
// Inefficient: O(n²) complexity
hosts := ""
for i := 0; i < replicas; i++ {
hosts = hosts + hostName + "." + subdomain + ","
}
// Efficient: O(n) complexity
var builder strings.Builder
for i := 0; i < replicas; i++ {
builder.WriteString(hostName)
builder.WriteString(".")
builder.WriteString(subdomain)
if i < replicas-1 {
builder.WriteString(",")
}
}
When adding new configuration options, carefully consider whether features should be enabled by default. Require explicit justification for default enablement, weighing functionality benefits against compatibility risks and user impact.
Key considerations:
Example from Helm values configuration:
custom:
# Don't enable by default - requires k8s v1.31+ APIs
# feature_gates: DynamicResourceAllocation=true
feature_gates: ""
# Enable capability by default - core functionality
root_queue:
capability: true
When in doubt, prefer conservative defaults that maintain backward compatibility, and provide clear documentation on how users can enable advanced features when needed.
When evolving APIs, prioritize backward compatibility and avoid changing the semantics of existing fields. Instead of overloading existing fields with new meanings, introduce new fields to maintain compatibility with existing usage patterns.
Key principles:
Example from the codebase:
// Instead of changing semantics of existing credentialName field:
if strings.HasSuffix(name, SdsCaSuffix) {
// This changes behavior for existing users
}
// Better approach - add a new field:
type TLSOptions struct {
CredentialName string // Keep existing semantics
CaCertCredentialName string // New field for CA certificates
}
When updating to new API versions, ensure field type changes (like *string to string) are handled correctly and check for both nil pointers and empty strings as appropriate. Always verify that changes follow the official API specification rather than making assumptions about field behavior.
When you identify duplicate code blocks, complex logic that can be simplified, or functionality that doesn’t belong in its current location, extract it into well-named, reusable functions. This improves code organization, reduces duplication, and enhances readability.
Key scenarios to apply this practice:
Example transformation:
// Before: Complex inline logic
if req.Event != busv1alpha1.PodPendingEvent {
cc.delayActionMapLock.Lock()
if taskMap, exists := cc.delayActionMap[key]; exists {
for podName, delayAct := range taskMap {
// 20+ lines of complex cancellation logic
}
}
cc.delayActionMapLock.Unlock()
}
// After: Extracted function
if req.Event != busv1alpha1.PodPendingEvent {
cc.cancelDelayedActions(key, req)
}
func (cc *jobcontroller) cancelDelayedActions(key string, req *Request) {
cc.delayActionMapLock.Lock()
defer cc.delayActionMapLock.Unlock()
// Clear, focused cancellation logic
}
This practice makes code more maintainable, testable, and easier to understand by giving complex operations descriptive names and clear boundaries.
When documenting algorithms, provide detailed explanations of the underlying logic with concrete examples and clear process flows. Abstract algorithmic descriptions should be accompanied by specific scenarios that illustrate how the algorithm behaves in practice.
Key requirements:
Example of good algorithm documentation:
## Resource Allocation Algorithm
### Problem
Different resource types require different allocation strategies to optimize cluster utilization.
### Solution
The ResourceStrategyFit algorithm applies different strategies per resource type:
1. **CPU resources**: Use LeastRequestedPriority to disperse tasks and avoid hotspots
2. **GPU resources**: Use MostRequestedPriority to aggregate tasks and reduce fragmentation
### Process Flow
1. Identify resource types in the pod specification
2. For each resource type, apply the configured strategy:
- If CPU: Calculate dispersion score = (capacity - allocated) / capacity
- If GPU: Calculate aggregation score = allocated / capacity
3. Combine scores using weighted priorities
This approach ensures that algorithmic concepts are accessible to both implementers and reviewers, reducing confusion and enabling better code quality.
Functions should return errors explicitly to callers rather than terminating the program, hiding errors in boolean returns, or using implicit error handling. This allows higher-level code to make appropriate decisions about error handling, recovery, and program flow.
Key principles:
return fmt.Errorf(...) instead of log.Fatalf() or t.Fatalf()Example of problematic pattern:
func fileExists(filename string) bool {
_, err := os.Stat(filename)
if err != nil && !os.IsNotExist(err) {
log.Warnf("Unexpected error checking file %s: %v", filename, err)
}
return err == nil
}
func initBpfObjects() {
if err := os.Mkdir(MapsPinpath, os.ModePerm); err != nil {
log.Fatalf("unable to create ambient bpf mount directory: %v", err)
}
}
Improved pattern:
func fileExists(filename string) (bool, error) {
if filename == "" {
return false, nil
}
_, err := os.Stat(filename)
if err != nil && !os.IsNotExist(err) {
return false, fmt.Errorf("unexpected error checking file %s: %v", filename, err)
}
return err == nil, nil
}
func initBpfObjects() error {
if err := os.Mkdir(MapsPinpath, os.ModePerm); err != nil {
return fmt.Errorf("unable to create ambient bpf mount directory: %v", err)
}
return nil
}
This approach provides callers with the flexibility to implement appropriate error handling strategies, retry logic, or graceful degradation based on their specific context and requirements.
Names should accurately reflect their actual functionality and behavior. Avoid misleading or vague identifiers that don’t match what the code actually does.
Key principles:
allowOverwrite instead of Rotation when not doing true rotation)IsNetworkGateway instead of IsGateway to differentiate from other gateway types)WithService instead of AddService; if it updates cache, name it accordingly rather than GetPublicKeyKey() instead of GetIstioEndpointKey() when called on an IstioEndpoint)clt.Config().Service() instead of "a")k in variable names (annotation instead of kAnnotation)Example:
// Poor: misleading name
func (ep *IstioEndpoint) GetIstioEndpointKey() string { ... }
// Better: concise and clear
func (ep *IstioEndpoint) Key() string { ... }
// Poor: doesn't reflect actual behavior
func AddService(service *Service) WasmPluginListenerInfo { ... }
// Better: indicates non-mutating operation
func WithService(service *Service) WasmPluginListenerInfo { ... }
Identify and eliminate computationally expensive operations in frequently executed code paths. Common expensive operations include repeated computations, unbounded API calls, inefficient string operations, and operations that don’t scale with data size.
Key strategies to avoid expensive operations:
Example optimization for string building:
// Expensive: fmt.Sprintf with multiple allocations (317ns, 5 allocs)
func getEndpointKey(portName string, portNum int32, ips []string) string {
ipString := strings.Join(ips, ", ")
return fmt.Sprintf("%s-%s-%d", ipString, portName, portNum)
}
// Optimized: strings.Builder with fewer allocations (75ns, 2 allocs)
func getEndpointKey(portName string, portNum int32, ips []string) string {
var b strings.Builder
for k, ip := range ips {
if k > 0 {
b.WriteString(", ")
}
b.WriteString(ip)
}
b.WriteString("-")
b.WriteString(portName)
b.WriteString("-")
b.WriteString(strconv.Itoa(int(portNum)))
return b.String()
}
Always consider the performance impact of operations in hot paths and prefer efficient alternatives that scale well with system load.
Ensure network-related configurations remain consistent across different components and avoid unintended overwrites that could break connectivity or security.
When modifying network configurations, always consider the impact on related components and use appropriate update mechanisms:
// Use patch or SSA to merge configurations patchBytes, err = c.generateShadowServicePatch(existingService, service) _, err = svcClient.Patch(existingService.Name, existingService.Namespace, types.MergePatchType, patchBytes)
2. **Maintain consistency in DNS and network settings** across bootstrap and dynamic configurations:
```go
// Ensure DNS lookup family settings match between bootstrap and cluster builder
if networkutil.AllIPv4(cb.proxyIPAddresses) {
c.DnsLookupFamily = cluster.Cluster_V4_ONLY
} else if networkutil.AllIPv6(cb.proxyIPAddresses) {
c.DnsLookupFamily = cluster.Cluster_V6_ONLY
} else {
// Dual Stack - use consistent logic across components
c.DnsLookupFamily = cluster.Cluster_ALL
}
// Merge HTTP1 options to avoid overwriting existing settings
setHTTP1Options(cluster, effectiveProxyConfig.GetProxyHeaders().GetPreserveHttp1HeaderCase())
This prevents network connectivity issues, security vulnerabilities, and configuration drift that can occur when components have inconsistent network settings or when updates inadvertently overwrite critical configurations.
Choose variable, function, and method names that clearly communicate their purpose and behavior. Avoid ambiguous abbreviations, numbered suffixes, double negatives, and names that don’t accurately reflect functionality.
Key principles:
preemptorPodPriority instead of podPriority, candidateHyperNodes instead of reScoreHyperNodesok instead of ok1nodeIsReady instead of !nodeIsNotReadyfilterVictimsFn not preemptable_reclaimable_fn)checkNodeGPUSharingPredicateWithScoreExample:
// Poor naming
func (pmpt *Action) taskEligibleToPreemptOthers(preemptor *api.TaskInfo) (bool, string) {
podPriority := PodPriority(preemptor.Pod) // Unclear whose priority
// ...
}
// Better naming
func (pmpt *Action) taskEligibleToPreemptOthers(preemptor *api.TaskInfo) (bool, string) {
preemptorPodPriority := PodPriority(preemptor.Pod) // Clear ownership
// ...
}
Clear naming reduces cognitive load, prevents misunderstandings, and makes code self-documenting for future maintainers.
Always add explicit nil checks before accessing object properties and after type assertions to prevent null pointer exceptions and runtime panics. Even when type assertions succeed, the underlying value can still be nil, requiring additional validation.
Key scenarios requiring nil checks:
Example from the discussions:
// Before: Unsafe type assertion
if devices, ok := node.Others[val].(api.Devices); ok {
code, msg, err := devices.FilterNode(task.Pod) // Potential panic if devices is nil
}
// After: Safe with nil check
if dev, ok := node.Others[val].(api.Devices); ok {
if dev == nil {
continue // or handle appropriately
}
code, msg, err := dev.FilterNode(task.Pod)
}
// Before: Unsafe property access
func isNodeUnschedulable(node *v1.Node) bool {
return node.Spec.Unschedulable // Potential panic if node is nil
}
// After: Safe with nil check
func isNodeUnschedulable(node *v1.Node) bool {
if node == nil {
return false
}
return node.Spec.Unschedulable
}
This practice prevents runtime crashes and makes code more robust, especially when dealing with interface types and optional parameters.
Ensure naming consistency across related components and throughout the codebase to prevent developer confusion and maintenance issues. When similar concepts use different naming variations, choose one pattern and apply it consistently.
Key principles:
Example from the codebase:
# Inconsistent - causes confusion
metadata:
name: mtls-serve # directory uses "serve"
spec:
containers:
- name: mtls-serve
image: localhost:5000/mtls-server:latest # image uses "server"
# Consistent - clear and predictable
metadata:
name: mtls-server
spec:
containers:
- name: mtls-server
image: localhost:5000/mtls-server:latest
When updating legacy naming, prioritize consistency with the broader system over maintaining alignment with individual objects. This reduces cognitive load and prevents the type of confusion that “is going to trip someone up one day.”
Eliminate duplicate computations, unnecessary API calls, and redundant processing to improve performance and reduce system load. This includes avoiding duplicate function calls, minimizing API server requests, and implementing early returns or caching to prevent redundant work.
Key strategies:
Example from the discussions:
// Bad: Calling calculateWeight twice
hyperNodeTierWeight: calculateWeight(arguments),
taskNumWeight: calculateWeight(arguments),
// Good: Call once and reuse
weight := calculateWeight(arguments)
hyperNodeTierWeight: weight,
taskNumWeight: weight,
// Bad: Always calculating expensive operation
minMember := pg.getMinMemberFromUpperRes(pod) // Heavy API operation
if err := pg.createNormalPodPGIfNotExist(pod); err != nil {
// Good: Only calculate when needed
if err := pg.createNormalPodPGIfNotExist(pod); err != nil {
// Only calculate minMember if podgroup creation is needed
minMember := pg.getMinMemberFromUpperRes(pod)
}
This approach reduces computational overhead, decreases API server load, and improves overall system performance, especially in large-scale environments where these optimizations compound significantly.
Always protect shared state access and ensure proper synchronization in concurrent operations to prevent race conditions. Key areas to review:
// Bad: Mutating shared slice in-place
func (ep *IstioEndpoint) SortAddresses() []string {
sort.Sort(sort.Reverse(sort.StringSlice(ep.Addresses))) // Race condition!
return ep.Addresses
}
// Good: Create copy or use proper locking
func (ep *IstioEndpoint) SortAddresses() []string {
ep.mu.Lock()
defer ep.mu.Unlock()
addrSlice := make([]string, len(ep.Addresses))
copy(addrSlice, ep.Addresses)
sort.Sort(sort.Reverse(sort.StringSlice(addrSlice)))
return addrSlice
}
Event Processing Order: When multiple informers or event handlers process the same resources, ensure proper ordering or make handlers idempotent to handle events arriving in any order.
Context Cancellation: Always respect context cancellation in long-running operations, especially during sleep or wait periods:
// Bad: Sleep blocks context cancellation
time.Sleep(next)
// Good: Respect context during sleep
select {
case <-time.After(next):
case <-ctx.Done():
return ctx.Err()
}
Lock Ordering: When acquiring multiple locks or waiting for synchronization, establish clear ordering to prevent deadlocks. Consider whether locks should be acquired before or after waiting for external conditions.
Test Flakiness: If tests are flaky due to timing issues, make them more robust by allowing for multiple valid event sequences rather than expecting exact ordering, especially in distributed systems where event coalescing can occur.
Always check and handle errors returned from function calls, especially API operations, before proceeding with subsequent logic. Unchecked errors can lead to inconsistent state, data corruption, or unexpected behavior.
Key practices:
Example of missing error check:
// Bad: Error not checked
_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})
// Continuing without checking err can cause issues
// Good: Error properly handled
_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})
if err != nil {
return err
}
Example of proper error handling with state management:
// Check error before updating state variables
if err := sc.executePreBind(ctx, bindContext); err != nil {
// Handle error and ensure cleanup/resync occurs
sc.resyncTask(bindContext.TaskInfo)
return err
}
// Only proceed with state updates after successful operation
This practice prevents silent failures, ensures consistent error propagation, and maintains system reliability by catching issues early in the execution flow.
Eliminate timing dependencies in tests by using proper retry mechanisms and assertions instead of sleep statements. Sleep-based waits make tests flaky and unreliable, while retry mechanisms with conditions provide more robust test execution.
Replace time.Sleep() calls with assert.EventuallyEqual() or appropriate retry functions that wait for specific conditions to be met. Choose retry.Converge() over retry.MaxAttempts() when you need consistent state verification, as it waits for multiple consecutive successful checks rather than potentially giving up early during system startup.
Example of problematic code:
time.Sleep(2 * time.Second) // wait for the namespace to be created
Preferred approach:
assert.EventuallyEqual(t, func() int {
se := rig.se.Get("pre-existing", "default")
return len(autoallocate.GetAddressesFromServiceEntry(se))
}, 2, retry.Converge(10), retry.Delay(time.Millisecond*5))
This approach makes tests more reliable by waiting for actual conditions rather than arbitrary time periods, reducing flakiness and improving test execution speed.
Ensure all plugin configurations follow a consistent, structured format across the codebase to improve maintainability and user experience. Different plugins and features should not use ad-hoc configuration styles that make the system fragmented and difficult to understand.
Key principles:
binpackExample of good practice:
# Structured configuration approach
- name: resource-strategy-fit
arguments:
resourceStrategyFitWeight: 10
resources:
nvidia.com/gpu:
type: MostAllocated
weight: 2
cpu:
type: LeastAllocated
weight: 1
Example of what to avoid:
# Ad-hoc string-based configuration
arguments:
reserve.nodeLabel: label1,label2
reserve.define.label1: {"business_type": "ebook"}
Better alternative:
# Structured alternative
arguments:
reserveLabels:
- nodeSelector:
business_type: ebook
startHour: 3
endHour: 4
resources:
cpu: 32
memory: 64
This standardization reduces cognitive load for users, simplifies validation, and makes the configuration system more maintainable as new features are added.
When implementing security-related functionality such as certificate parsing, cryptographic validation, or authentication mechanisms, always document the expected behavior and include references to official documentation that explains the underlying security decisions.
This practice is crucial for several reasons:
Example:
// This cert has a negative serial number.
// Go should fail to parse it, but we should handle this gracefully
// Reference: https://golang.org/pkg/crypto/x509/#Certificate
// Go's x509 package rejects certificates with negative serial numbers
// as per RFC 5280 requirements
Always include links to relevant RFCs, official language documentation, or security standards when implementing security features. This documentation becomes invaluable during security reviews and helps establish the rationale behind security-related code decisions.
Write precise, unambiguous documentation that explains what fields and methods represent rather than implementation details. Avoid recursive definitions, ambiguous terms, and focus on the user-facing behavior and purpose.
Key principles:
Examples of improvements:
Instead of:
// Value defines how much of a certain device capacity is available.
// If the capacity is consumable, the consumed amount is deducted and cached in memory by the scheduler.
Write:
// Value defines the total amount of this capacity the device has.
// This field reflects the fixed total capacity and does not change.
Instead of:
// container name
Write:
// The name of the container requesting resources.
Instead of recursive definitions:
// Mixins defines the mixins available for devices and counter sets
Write:
// Mixins provides common definitions that can be used for devices and counter sets in the ResourceSlice.
This approach ensures documentation is immediately understandable without requiring knowledge of implementation details or other parts of the system.
Implement efficient search strategies that minimize computational overhead through proper ordering, early termination, and targeted scope reduction. This involves three key principles:
Strategic Ordering: Prioritize search candidates to increase likelihood of early success. For example, when allocating devices, place simpler options (without binding conditions) before complex ones to reduce allocation failures and retries.
Early Termination: Add explicit break statements or return conditions once the desired result is found, avoiding unnecessary iterations through remaining candidates.
Scope Reduction: Target searches to specific subsets rather than iterating through entire collections when the context allows for narrower scope.
Example of optimized device search with early termination:
// Before: searching all devices unnecessarily
for _, device := range slice.Spec.Devices {
if device.Name == internal.id.Device && len(device.Basic.BindingConditions) > 0 {
allocationResult.Devices.Results[i].BindingConditions = device.Basic.BindingConditions
// continues searching even after match found
}
}
// After: early termination once target found
for _, device := range slice.Spec.Devices {
if device.Name == internal.id.Device {
allocationResult.Devices.Results[i].BindingConditions = device.Basic.BindingConditions
break // stop searching once found
}
}
Consider implementing tiered allocation approaches where multiple fallback strategies are attempted in order of preference, allowing the algorithm to gracefully degrade while maintaining optimal performance for common cases.
Avoid exposing sensitive information through error messages, logs, configuration, or other output channels that might be accessible to unauthorized parties such as cluster administrators or operators.
When handling sensitive data like credentials, environment variables, or configuration files, ensure that:
Example of secure error handling:
// Instead of:
return "", fmt.Errorf("invalid environment variable format: %s", line)
// Use:
klog.Errorf("ParseEnv failed at line %d: %s", lineNum, line) // Log for debugging
return "", fmt.Errorf("invalid environment variable format at line %d", lineNum) // Safe error for user
This principle applies to environment variable parsing, credential management, configuration validation, and any feature that processes user-provided data that might contain secrets. Always consider who has access to error messages and logs before including potentially sensitive information.
Always wrap errors with meaningful context that explains what operation failed and why. Use proper error wrapping with fmt.Errorf("operation failed: %w", err) to preserve the original error chain while adding context. Include explanatory comments for complex error handling logic to clarify the intended behavior and recovery strategy.
When handling timeouts, wrap with specific timeout context:
if err != nil {
if errors.Is(err, context.DeadlineExceeded) {
err = fmt.Errorf("device binding timeout: %w", err)
}
return statusError(logger, err)
}
Add comments to explain error handling decisions:
// Returning an error here causes another scheduling attempt.
// In that next attempt, PreFilter will detect the timeout or
// error and try to recover.
return statusError(logger, err)
Make error messages descriptive and mention relevant context like retry attempts, duration, or affected resources. This helps with debugging and provides actionable information for both developers and operators.
Ensure comprehensive test coverage by systematically considering all scenarios, edge cases, and feature combinations when adding or modifying functionality. This includes testing both positive and negative cases, feature gate enabled/disabled states, error conditions, and boundary conditions.
When adding new functionality, ask yourself:
For example, when adding CEL changes, don’t just test the happy path - also test invalid expressions, edge cases in the evaluation logic, and integration with other components. When implementing backtracking algorithms, ensure tests cover scenarios where backtracking is actually triggered, not just cases where it succeeds on the first attempt.
// Good: Comprehensive test coverage
func TestAllocatorBacktracking(t *testing.T) {
testCases := []struct {
name string
// Test successful allocation
// Test backtracking scenarios
// Test resource exhaustion
// Test constraint violations
// Test feature gate enabled/disabled
}{
{
name: "successful allocation without backtracking",
// ...
},
{
name: "backtracking required due to constraint violation",
// ...
},
{
name: "backtracking fails - no valid allocation",
// ...
},
}
}
Incomplete test coverage often leads to production bugs that could have been caught during development. Take time to systematically think through all the ways your code could be exercised.
When implementing feature-gated fields, always preserve existing field values when the feature gate is disabled, but drop new field values that depend on disabled features. This prevents breaking existing functionality during feature gate transitions or version skew scenarios.
Key principles:
DefaultFeatureGate.Enabled() callsExample implementation:
func dropDisabledDRAFields(newSlice, oldSlice *resource.ResourceSlice) {
// Check if feature is enabled OR was already in use
if utilfeature.DefaultFeatureGate.Enabled(features.DRANewFeature) ||
draNewFeatureInUse(oldSlice) {
return
}
// Only drop fields if not previously used
for i := range newSlice.Spec.Devices {
newSlice.Spec.Devices[i].NewFeatureField = nil
}
}
// Separate validation for new features
if !utilfeature.DefaultFeatureGate.Enabled(features.DRANewFeature) &&
!newFeatureInUse(oldPodSpec, oldPodStatus) {
// Drop new feature fields
dropNewFeatureFields(podSpec)
}
This approach ensures backward compatibility, prevents data loss during upgrades/downgrades, and maintains clean separation between feature-gated and stable functionality.
Choose names that accurately reflect the actual functionality, purpose, or semantic meaning rather than using generic, misleading, or implementation-focused terms. Names should be self-documenting and help readers understand what the code does without needing additional context.
Key principles:
Examples:
// Bad: Generic and doesn't explain what it does
func initialize() error
// Good: Describes the actual purpose
func migrateRecordVersions() error
// Bad: Misleading - suggests "at least" semantics
type CapacityRequirements struct {
Minimum map[QualifiedName]resource.Quantity
}
// Good: Clear about guaranteed allocation
type CapacityRequirements struct {
Requests map[QualifiedName]resource.Quantity
}
// Bad: Generic variable name
bound, err := isClaimReadyForBinding(claim)
// Good: Semantic meaning is clear
ready, err := isClaimReadyForBinding(claim)
// Bad: Overloaded term "binding" used imprecisely
func isClaimBound(claim *ResourceClaim) bool
// Good: Precise about what condition is being checked
func isClaimReadyForBinding(claim *ResourceClaim) bool
This approach improves code readability and reduces the cognitive load on developers trying to understand the codebase.
Prioritize code simplification by removing unnecessary complexity, avoiding duplication, and leveraging existing utilities. This improves readability and maintainability while reducing potential bugs.
Key practices:
Example of simplification:
// Before: Unnecessary length check and duplication
if len(request.FirstAvailable) > 0 {
for _, subRequest := range request.FirstAvailable {
// process subRequest
}
}
// After: Direct ranging (safe for empty slices)
for _, subRequest := range request.FirstAvailable {
// process subRequest
}
// Before: Local variables for simple values
trueVal := true
spec.SetHostnameAsFQDN = &trueVal
// After: Use utility functions
spec.SetHostnameAsFQDN = ptr.To(true)
This approach reduces cognitive load, minimizes maintenance overhead, and makes code more self-documenting by removing unnecessary abstractions and leveraging well-tested standard patterns.
API fields must be comprehensively documented with clear behavior specifications, format constraints, and interaction details. This includes documenting how fields interact with each other, specifying allowed formats and validation rules, clarifying field semantics and placement decisions, and aligning naming conventions with existing Kubernetes APIs.
Key documentation requirements:
Count when Count is > 1 … are these capacityRequests per device or across devices?”Example of proper field documentation:
// CapacityRequests define resource requirements against each capacity.
// When Count > 1, these requirements apply per device.
// When allocationMode=All, this selects all matching devices which can satisfy the capacityRequests.
//
// +optional
// +featureGate=DRAConsumableCapacity
CapacityRequests *CapacityRequirements
// ExtendedResourceName is the extended resource name for devices of this class.
// Must be a valid extended resource name format (domain/resource-name).
// Only one DeviceClass should specify the same extendedResourceName.
// If multiple DeviceClasses specify the same name, scheduling behavior is undefined.
//
// +optional
// +featureGate=DRAExtendedResources
ExtendedResourceName *string
This practice prevents confusion during API evolution, ensures consistent behavior across implementations, and provides clear guidance for API consumers.
Optimize performance by avoiding unnecessary computations, allocations, and operations. Use feature gates, lazy evaluation, and conditional logic to prevent work that won’t be used.
Key strategies:
Example of feature-gated allocation:
// Avoid allocating when feature is disabled
var shareIDs []structured.SharedDeviceID
var deviceCapacities []structured.DeviceConsumedCapacity
if a.enabledConsumableCapacity {
shareIDs = make([]structured.SharedDeviceID, 0, 20)
deviceCapacities = make([]structured.DeviceConsumedCapacity, 0, 20)
}
Example of conditional processing:
// Performance optimization: skip the for loop if the feature is off
if a.features.DeviceBinding {
// Only do expensive work when needed
for _, device := range devices {
// ... expensive operations
}
}
This approach is particularly important in hot paths, validation code that runs frequently, and resource allocation scenarios where unnecessary work can accumulate significant overhead.
Ensure feature gates are properly managed across all configuration files and contexts. Feature-specific permissions, settings, and resources should be conditionally included based on appropriate feature gate guards rather than being present in default configurations. Avoid manually adding feature gates to configuration files when they are automatically managed by the system, as this can interfere with jobs that need to run with different feature combinations.
For beta and GA features, follow established patterns: beta features should typically be enabled by default, while alpha features remain disabled. When feature gates eventually get removed for GA features, replace feature gate references in test names and descriptions with plain text equivalents.
Example of proper feature gate guarding:
# Good: Guard DRA-specific permissions behind feature gate
- apiGroups:
- resource.k8s.io
resources:
- devicetaintrules
verbs: [get, list, watch]
# This should be guarded on DRADeviceTaints feature
# Avoid: Don't manually add feature gates when auto-managed
featureGates:
DynamicResourceAllocation: true
# DRAExtendedResource: true # Let alpha job add this automatically
This ensures configurations remain clean, maintainable, and compatible with different deployment scenarios while preventing conflicts between manual and automatic feature gate management.
Always measure and validate performance impact through benchmarking before making changes that could affect system performance, even when changes appear to be obvious optimizations. Removing seemingly redundant configurations or adding new features can have unexpected performance consequences that only surface during integration testing or production use.
Before making performance-related changes:
Example from resource configuration:
resources:
requests:
foo.com/bar-{{.Index}}: 1
limits:
foo.com/bar-{{.Index}}: 1 # Don't remove without testing - may be required for non-overcommitable resources
Performance validation should include both automated benchmarks and integration tests to catch issues like “Limit must be set for non overcommitable resources” that only appear in specific runtime conditions. Document performance comparison results with statistical significance testing to make informed decisions about optimization trade-offs.
For security-sensitive operations like shell argument parsing, input validation, or cryptographic functions, prefer copying minimal, well-understood code over adding external dependencies, especially when:
This approach reduces attack surface, eliminates supply chain risks, and ensures long-term maintainability of security-critical code paths.
Example: Instead of adding a new dependency for shell argument splitting:
// Prefer: Copy the specific function needed
func splitShellArgs(s string) ([]string, error) {
// Implementation copied and audited locally
}
// Avoid: Adding dependency with unclear maintenance
import "github.com/unknown-maintainer/shlex"
When the functionality is minimal and security-sensitive, the maintenance burden of copied code is often preferable to the risks of external dependencies.
Ensure observability mechanisms (events and metrics) are implemented correctly with appropriate types, accurate labeling, and user-focused messaging. Use events for user-visible information that appears in kubectl describe, not just internal logging. Choose correct event types (EventTypeNormal for waiting states, EventTypeWarning for actual failures) and provide consistent, descriptive messages that identify relevant resources like nodes and devices. Avoid duplicate events by checking if they’re already being emitted elsewhere in the flow. For metrics, use precise labels that match the actual operation context and employ direct metric operations (Inc/Dec) when appropriate.
Example of proper event usage:
// Good: Informational event for waiting state
pl.fh.EventRecorder().Eventf(claim, pod, v1.EventTypeNormal, "BindingConditionsPending", "Scheduling",
"waiting for driver to report status for device %s/%s/%s on node %s.",
deviceRequest.Driver, deviceRequest.Pool, deviceRequest.Device, nodeName)
// Good: Proper metric labeling
f.metricsRecorder.ObservePluginDurationAsync(metrics.PreBindPreFlight, pl.Name(), status.Code().String(), metrics.SinceInSeconds(startTime))
This ensures users can effectively troubleshoot issues through kubectl describe while maintaining accurate system metrics for monitoring and debugging.
Use early return patterns with null checks to reduce nesting and improve code readability. Avoid unnecessary null checks for values that are guaranteed to be non-nil, and prefer length checks over nil checks for slices.
When checking for null values, use early returns to handle the null case first, then continue with the main logic. This pattern reduces cognitive load and prevents deeply nested conditional blocks.
For optional fields that are feature-gated or alpha, avoid checking default values alongside null checks - the null check alone is sufficient.
Example patterns:
// Good: Early return pattern
if result.ShareID == nil {
allocatedDevices.Insert(deviceID)
continue
}
sharedDeviceID := structured.MakeSharedDeviceID(deviceID, *result.ShareID)
allocatedSharedDeviceIDs.Insert(sharedDeviceID)
// Good: Simplified null equality check
if statusA == nil || statusB == nil {
return statusA == statusB
}
// Good: Length check instead of nil check for slices
if len(bindingConditions) == 0 && len(bindingFailureConditions) > 0 {
// handle error case
}
// Good: Simple null check for optional fields
if device.ShareID != nil {
// process ShareID
}
// Avoid: Unnecessary null checks for guaranteed non-nil values
if f.handle != nil && hasExtendedResource { // f.handle is always non-nil
This approach makes null handling explicit, reduces the chance of null reference errors, and creates more maintainable code by clearly separating null handling from business logic.
When defining RBAC rules and authorization policies, grant only the minimum necessary permissions required for the intended functionality. Avoid copying broad permission sets or including unnecessary verbs that expand the attack surface.
Review authorization configurations to ensure they follow the principle of least privilege. For each resource and verb combination, verify that the permission is actually needed for the component’s operation.
Example from Kubernetes RBAC:
# Instead of granting multiple unnecessary verbs:
- apiGroups: [""]
resources: ["pods/finalizers"]
verbs: ["get", "list", "patch", "update", "watch"] # Too broad
# Grant only what's actually needed:
- apiGroups: [""]
resources: ["pods/finalizers"]
verbs: ["update"] # Only what's meaningful for this use case
This practice reduces security risk by limiting the scope of potential privilege escalation and ensures that components cannot perform unintended operations even if compromised.
Configuration examples in documentation must be complete and reference authoritative sources to prevent deployment issues and developer confusion. Incomplete configuration lists can lead to runtime errors and failed setups.
When documenting configuration:
Example of good practice:
# Example api-server.env file (partial - see Procfile for complete list)
ARGOCD_BINARY_NAME=argocd-server
ARGOCD_FAKE_IN_CLUSTER=true
KUBECONFIG=/Users/<YOUR_USERNAME>/.kube/config # Must be absolute path
...
# For complete environment variables, refer to the Procfile:
# https://github.com/argoproj/argo-cd/blob/master/Procfile
Always include notes about:
This prevents users from encountering “incomplete list of env variables” errors and ensures reliable configuration setup across different environments.
When configuring networking components, prefer stable and permissive defaults over restrictive or experimental configurations that may not work across all environments. This approach ensures broader compatibility and reduces the risk of breaking existing deployments.
Key principles:
Example from NetworkPolicy configuration:
egress:
# Allow all egress traffic by default rather than restrictive rules
- {}
This approach prioritizes system stability and broad compatibility over maximum security or cutting-edge features, allowing users to customize and lock down configurations based on their specific requirements and environments.
Avoid copying or reimplementing production logic in test files. Instead, refactor the production code to extract small, testable functions that can be unit tested with mocked dependencies. Tests should call the actual production functions rather than duplicating their logic.
When you find yourself copying complex logic from production code into tests, this indicates the production code needs refactoring. Extract the core logic into smaller, focused functions that accept dependencies as parameters, making them easier to test in isolation.
For example, instead of reimplementing import logic in a test:
// Bad: Reimplementing logic in test
func Test_importResources(t *testing.T) {
// ... lots of copied logic from backup.go
if bakObj.GetKind() == "Secret" {
dynClient = dynamicClient.Resource(secretResource).Namespace(bakObj.GetNamespace())
}
// ... more copied logic
}
// Good: Extract testable function and test it
func (opts *importOpts) executeImport(client dynamic.Interface) error {
// extracted logic here
}
func Test_executeImport(t *testing.T) {
fakeClient := dynamicfake.NewSimpleDynamicClient(scheme)
opts := &importOpts{...}
err := opts.executeImport(fakeClient)
// verify results by querying the fake client
}
This approach creates more maintainable tests, reduces code duplication, and ensures tests actually validate the production code rather than a separate implementation.
Choose names that accurately reflect the behavior and purpose of code elements. Names should be semantically precise and avoid misleading implications. This applies to methods, variables, and configuration parameters.
Key guidelines:
Examples:
// Misleading - implies just getting indexes
func getIndexes(k string, indexes []int)
// Better - accurately describes behavior of populating the slice
func fillIndexes(k string, indexes []int)
// Misleading - implies attaching something
func attachRateLimiter(config *Config) RateLimiter
// Better - accurately describes creating a new instance
func newDefaultRateLimiter() RateLimiter
// Misleading - implies complete draining
func drainBuffer(items []Item)
// Better - accurately describes partial processing
func processBuffer(items []Item)
When designing APIs, prioritize long-term stability and compatibility to avoid breaking changes. Use request/response objects pattern for methods that might evolve over time rather than direct parameters:
// Avoid - difficult to extend without breaking changes
func Terminate(ctx MutableContext, identity, reason string, details *commonpb.Payloads) error
// Prefer - allows adding fields without breaking existing clients
func Terminate(ctx MutableContext, request TerminateComponentRequest) (TerminateComponentResponse, error)
Be cautious when using protocol buffer messages in APIs, particularly with complex structures or oneoff fields that may cause serialization issues:
// There are known issues with the default protobuf json converter when the message
// contains oneoff fields - https://protobuf.dev/programming-guides/json/
// Consider using protojson to explicitly serialize/deserialize in such cases:
import "google.golang.org/protobuf/encoding/protojson"
// Explicit serialization for complex types
jsonBytes, err := protojson.Marshal(myProtoMessage)
Design semantically meaningful return types that reflect the logical structure of your data rather than implementation details:
// Avoid returning raw indices
func getApproximateBacklogCount(subqueue int) int64
// Prefer returning a map keyed by meaningful values
func getApproximateBacklogCounts() map[int32]int64
When exposing interfaces, carefully consider which methods should be public. Expose only what external consumers need while keeping implementation details hidden to maintain flexibility for internal changes.
Prioritize code readability by using clearer control structures, extracting complex expressions into descriptive variables, and choosing simpler syntax when available. This improves maintainability and makes code easier to understand for other developers.
Key practices:
<></> instead of <React.Fragment> when keys aren’t neededExample of improved readability:
// Instead of multiple if-else chains:
if (type === 'git' || type === 'oci') {
if (source.hasOwnProperty('chart')) {
source.path = source.chart;
delete source.chart;
source.targetRevision = 'HEAD';
}
} else if (type === 'helm') {
if (source.hasOwnProperty('path')) {
source.chart = source.path;
delete source.path;
source.targetRevision = '';
}
}
// Use switch statement:
switch (type) {
case 'git':
case 'oci':
if ('chart' in source) {
source.path = source.chart;
delete source.chart;
source.targetRevision = 'HEAD';
}
break;
case 'helm':
if ('path' in source) {
source.chart = source.path;
delete source.path;
source.targetRevision = '';
}
break;
}
// Extract complex expressions:
// Instead of inline complex logic:
const selectedApp = apps.find(app => AppUtils.appQualifiedName(app, useAuthSettingsCtx?.appsInAnyNamespaceEnabled) === val);
// Use descriptive temporary variable:
const qualifiedName = AppUtils.appQualifiedName(app, useAuthSettingsCtx?.appsInAnyNamespaceEnabled);
const selectedApp = apps?.find(app => qualifiedName === val);
When developing complex systems like schedulers or resource managers, create simulation-based testing frameworks to validate functionality in scenarios that cannot be easily reproduced in development environments. This approach enables testing of hardware configurations you don’t have access to (GPU/NPU nodes), large-scale cluster behaviors, and parameter changes before production deployment.
Simulation testing should provide visibility into system decision-making processes and support regression testing. For example, when testing a scheduler simulator, ensure it can show “how many nodes are there, which nodes are filtered for what reason, and how is each node scored” to enable thorough validation of scheduling logic.
Consider implementing simulation frameworks that:
This testing approach is particularly valuable for validating the impact of configuration changes, such as “after change the mostrequested.weight, if the average wait time of big task is shorter than before.”
Reduce cognitive load and improve code readability by minimizing nested code blocks. Prefer early returns and flattened logic over deeply nested conditions. This makes the code easier to read, understand, and maintain.
Example - Instead of nested conditions:
func foo() {
result, err = tryA()
if err != nil {
result, err = tryB()
if err != nil {
result, err = tryC()
if err != nil {
return nil, custom_error
}
}
}
return result, err
}
Prefer flattened logic:
func foo() {
result, err = tryA()
if err == nil {
return result, nil
}
result, err = tryB()
if err == nil {
return result, nil
}
result, err = tryC()
if err == nil {
return result, nil
}
return nil, custom_error
}
Key benefits:
Note: While reducing nesting is generally beneficial, consider maintaining clear logical grouping when it helps tell the code’s story. The goal is to balance readability with logical clarity.
Always consider computational complexity and performance when implementing algorithms. Look for opportunities to optimize through early exits, efficient comparison strategies, and avoiding nested loops that create O(N*M) complexity.
Key optimization techniques:
Example of optimized comparison:
// Instead of string concatenation comparison:
return fmt.Sprintf("%s/%s/%s", left.Type, left.Message, left.Status) <
fmt.Sprintf("%s/%s/%s", right.Type, right.Message, right.Status)
// Use field-by-field comparison with early returns:
if left.Type != right.Type {
return left.Type < right.Type
}
if left.Message != right.Message {
return left.Message < right.Message
}
return left.Status < right.Status
Example of early loop exit:
// Add break to avoid unnecessary iterations
for _, r := range resources {
if condition_met {
bAllNeedPrune = false
break // Exit early once condition is found
}
}
Consider the algorithmic impact of your implementation choices, especially when dealing with collections or repeated operations that could affect system performance at scale.
Functions should have thorough documentation that follows Go conventions and explains both the “what” and “why” of the code. This includes:
Proper Go doc format: Start function comments with the function name, e.g., “GetSecretByName returns the Secret…” instead of “Returns the Secret…”
Explain complex logic: Add explanatory comments for non-obvious code sections that describe why the logic exists, not just what it does.
Document all parameters and return values: Provide clear descriptions of what each parameter represents and what the function returns, especially for complex functions with multiple return values.
Include context and intent: Explain the purpose and use cases of the function, particularly for complex business logic.
Example of good documentation:
// alreadyAttemptedSync is meant to help the caller understand whether an identical sync operation
// has been attempted, to avoid excessively retrying the exact same sync operation.
//
// alreadyAttemptedSync returns true if either 1) newRevisionHasChanges is true and the most recently
// synced revision(s) exactly match the given desiredRevisions, 2) newRevisionHasChanges is false and
// the most recently synced app source configuration matches exactly the current app source configuration,
// or 3) the most recent operation state is missing a sync result but the sync phase is completed.
//
// TODO: remove the last two return parameters, since they're effectively just aliases for fields
// on the app object.
func alreadyAttemptedSync(app *appv1.Application, desiredRevisions []string, newRevisionHasChanges bool) (bool, []string, synccommon.OperationPhase) {
// If pruning is enabled and the app is *not* allowed to have an empty desired state,
// we need to ensure we're not about to accidentally wipe out all resources.
// This is a safety mechanism to prevent full deletion due to automation errors (e.g., empty Git path).
if app.Spec.SyncPolicy.Automated.Prune && !app.Spec.SyncPolicy.Automated.AllowEmpty {
// ... implementation
}
}
Well-documented code reduces cognitive load for reviewers and future maintainers, making the codebase more accessible and maintainable.
When implementing algorithms that involve multiple operations (like iteration, validation, and execution), the order of operations can critically affect correctness. Always validate operation ordering to ensure no unintended side effects or lost data.
Key considerations:
Example of correct iterator usage:
// Good: Check boundary before consuming iterator
for b := 0; b < batchSize && it.Next(); b++ {
// Process item
}
// Bad: May consume item that gets dropped
for b := 0; it.Next() && b < batchSize; b++ {
// Process item
}
For validation/execution sequences:
// Good: Interleave validation and execution
for _, task := range tasks {
valid, err := validateTask(task)
if err != nil || !valid {
continue
}
err = executeTask(task) // Execute immediately after validation
}
// Bad: Separate validation from execution
validTasks := []Task{}
for _, task := range tasks {
valid, _ := validateTask(task)
if valid {
validTasks = append(validTasks, task)
}
}
for _, task := range validTasks {
executeTask(task) // Task may be invalid now due to previous executions
}
Favor modern React patterns over legacy approaches to improve code maintainability and prepare for future React upgrades. Use functional components with hooks instead of class components, and prefer React.useContext over the <Context.Consumer> render prop pattern to reduce JSX nesting and improve readability.
Legacy patterns like contextTypes can become blockers when upgrading React versions, so migrating to modern alternatives should be prioritized. When refactoring, replace class component instance variables with useRef for mutable references.
Example transformation:
// Instead of this legacy pattern:
<Consumer>
{ctx => (
<div>{/* component content */}</div>
)}
</Consumer>
// Use this modern approach:
const ctx = useContext(Context);
return <div>{/* component content */}</div>;
Consider adding linter rules to enforce these patterns consistently across the codebase, such as preferring useContext over Consumer components.
Ensure API parameter documentation clearly specifies default values, valid options, error conditions, and precise requirements to prevent user confusion and integration failures. Ambiguous documentation leads to misunderstandings about expected behavior and can cause integration problems.
When documenting API parameters, always include:
Example of improved documentation:
--filter-fields strings A comma separated list of fields to display. If not specified, displays entire manifest. Valid values: field names separated by commas. (Optional)
pullRequestState: Additional filter for MRs by state. Default: "" (all states). Valid values: "", "opened", "closed", "merged", "locked" (Optional)
API scope: Must strictly include https://www.googleapis.com/auth/admin.directory.group.readonly. Using broader scopes like https://www.googleapis.com/auth/admin.directory.group will cause API failures.
This prevents user confusion about default behavior, error conditions, and integration requirements.
Code should include explanatory comments that help future maintainers understand complex logic, design decisions, field semantics, and non-obvious implementation details. Comments are especially important for:
Comments should follow Go conventions by being placed above the code they describe rather than inline, and should be detailed enough to explain both the “what” and “why” of the implementation.
Example of good explanatory commenting:
// WithServices marks multiple services as part of the selection criteria. This is used when we want to find **all** policies attached to a specific proxy instance, rather than scoped to a specific service. This is useful when using ECDS, for example, where we might have:
// * Each unique service creates a listener, and applies a policy selected by `WithService` pointing to ECDS
// * All policies are found, by `WithServices`, and returned in ECDS
func (p WorkloadPolicyMatcher) WithServices(services []*Service) WorkloadPolicyMatcher {
// implementation...
}
// Addresses contains the endpoint addresses. All elements must have the same metadata
// and represent the same logical endpoint across different network interfaces.
Addresses []string
This practice significantly improves code maintainability and reduces the cognitive load for developers working with unfamiliar codebases.
Use early returns and guard clauses to reduce nesting levels and improve code readability. Instead of deeply nested if-else structures, invert conditions and return early when possible. This pattern makes the main logic flow more apparent and reduces cognitive load.
Examples of this pattern:
// Instead of deep nesting:
func processData(data []string) error {
if len(data) > 0 {
if isValid(data) {
// main logic here
return process(data)
} else {
return errors.New("invalid data")
}
} else {
return errors.New("no data")
}
}
// Prefer early returns:
func processData(data []string) error {
if len(data) == 0 {
return errors.New("no data")
}
if !isValid(data) {
return errors.New("invalid data")
}
// main logic here
return process(data)
}
This approach is particularly beneficial when functions grow in size or when dealing with multiple validation conditions. It follows Go idioms and makes error handling more explicit while keeping the happy path unindented.
Replace hardcoded values with named constants from common packages or make values configurable through environment variables or flags. This improves maintainability, reduces magic numbers, and enables runtime configuration.
Examples of good practices:
common.SyncOptionSkipDryRunOnMissingResource instead of "SkipDryRunOnMissingResource=true"dialTime := env.ParseDurationFromEnv("DIAL_TIMEOUT", 30*time.Second)const defaultMaxChildren = 2Avoid hardcoding configuration values directly in the code. Instead, centralize them in common packages or make them configurable through environment variables, command-line flags, or configuration files. This allows for easier testing, deployment flexibility, and reduces the risk of inconsistencies across the codebase.
Follow these critical patterns when using locks to prevent deadlocks, resource leaks, and performance issues:
// Prefer
taskTrackerLock sync.RWMutex
// Over
taskTrackerMu sync.RWMutex
// Good func (s *Service) ProcessData() { response := s.client.Call() // Do I/O first
s.lock.Lock()
defer s.lock.Unlock()
// Only use lock for quick state updates } ```
if !timer.Stop() {
select {
case <-timer.C: // drain the channel if fired
default:
}
}
s.lock.Lock() defer s.lock.Unlock()
select { case <-ctx.Done(): return ctx.Err() case <-s.workChan: // Process work }
---
## avoid repeated expensive operations
<!-- source: istio/istio | topic: Algorithms | language: Go | updated: 2025-07-09 -->
Avoid performing expensive computational operations repeatedly when the same result can be achieved through better algorithm design or data structure choices. This includes avoiding sorting on each access, repeated string parsing, or complex comparisons that could be precomputed or cached.
Key principles:
- **Sort once, not on each access**: Instead of sorting data every time it's compared or accessed, sort during creation or maintain sorted order
- **Use appropriate data structures**: Choose data structures that naturally support your access patterns rather than forcing expensive operations on simpler structures
- **Precompute when possible**: For operations that will be repeated, consider precomputing results or using indexing
Example from the codebase:
```go
// Bad: Sorting on each equality check
func WorkloadInstancesEqual(first, second *WorkloadInstance) bool {
firtAddrs := first.Endpoint.SortIstioEndpointByAddresses() // Mutates and sorts every time
// ... comparison logic
}
// Better: Use a more efficient comparison or maintain sorted order
func WorkloadInstancesEqual(first, second *WorkloadInstance) bool {
return IsAddrsEqualIstioEndpoint(first.Endpoint, second.Endpoint) // Efficient comparison without sorting
}
Another example:
// Bad: String concatenation and splitting for indexing
workloadNetworkServiceIndex := krt.NewIndex[string, model.WorkloadInfo](GlobalWorkloads, "network;service", func(o model.WorkloadInfo) []string {
// Later requires: parts := strings.Split(i.Key, ";")
})
// Better: Use a proper struct for composite keys
type NetworkServiceKey struct {
Network string
Service string
}
This approach reduces computational complexity, improves performance, and makes code more maintainable by choosing algorithms and data structures that match the usage patterns.
Prioritize code simplicity and readability by leveraging standard library functions, improving control flow patterns, and eliminating unnecessary complexity.
Key practices:
Use standard library functions instead of reinventing functionality. For example, use strings.Join(fields, "\t") instead of custom joinWithTabs functions, or slices.Contains(slice, item) instead of manual loops.
Apply “exit early” patterns to reduce nesting and improve readability. Structure conditionals to handle error cases or special conditions first, then continue with the main logic: ```go // Instead of nested conditions if condition { // main logic here } else { return error }
// Use exit early if !condition { return error } // main logic here
3. **Eliminate superfluous code** such as redundant else clauses after return/continue statements, unnecessary variables when one suffices, and boolean literals in expressions (`if !flag` instead of `if flag == false`).
4. **Avoid unnecessary complexity** by removing redundant code after refactoring, consolidating duplicate logic, and keeping variable scope minimal.
These practices enhance code maintainability, reduce cognitive load for reviewers, and follow Go idioms for clean, readable code.
---
## Comprehensive test structure
<!-- source: volcano-sh/volcano | topic: Testing | language: Go | updated: 2025-07-08 -->
Tests should focus on comprehensive coverage of main logic flows rather than trivial functions, use established testing patterns like TestCommonStruct for end-to-end validation, and maintain proper table-driven test formatting for readability and maintainability.
Key principles:
1. **Focus on main logic**: Test the core scheduling processes and business logic rather than simple getter/setter functions
2. **Use established patterns**: Leverage existing test utilities like TestCommonStruct to verify complete workflows
3. **Proper table formatting**: Structure test tables with clear field alignment and consistent formatting
Example of well-structured test:
```go
tests := []struct {
name string
args args
want ResourceStrategyFit
}{
{
name: "test with cpu and memory resources",
args: args{framework.Arguments{
"ResourceStrategyFitPlusWeight": 10,
"resources": map[string]interface{}{
"cpu": map[string]interface{}{
"type": "MostAllocated",
"weight": 1,
},
"memory": map[string]interface{}{
"type": "LeastAllocated",
"weight": 2,
},
},
}},
want: ResourceStrategyFit{
ResourceStrategyFitWeight: 10,
Resources: map[v1.ResourceName]ResourcesType{
"cpu": {
Type: config.MostAllocated,
Weight: 1,
},
"memory": {
Type: config.LeastAllocated,
Weight: 2,
},
},
},
},
}
For integration tests, use the TestCommonStruct pattern to validate complete scheduling workflows rather than isolated unit tests.
Avoid hardcoded values and magic numbers in configuration-related code by extracting them as named constants. This improves code maintainability, readability, and makes configuration values easier to modify.
Hardcoded values make code difficult to understand and maintain. When configuration values, timeouts, or default settings are embedded directly in the code, it becomes unclear what these values represent and makes them harder to change consistently across the codebase.
Examples of what to extract:
// Bad: Magic numbers and hardcoded strings
if resourceStrategyFitPluginWeight <= 0 {
resourceStrategyFitPluginWeight = 10 // What does 10 represent?
}
data, ok := cm.Data["device-config.yaml"] // Hardcoded key
if !exist || nowTs-ts > 60 { // What does 60 seconds represent?
Good: Extract as named constants:
const (
DefaultResourceStrategyFitWeight = 10
DeviceConfigKey = "device-config.yaml"
TaskStatusUpdateIntervalSeconds = 60
)
// Usage
if resourceStrategyFitPluginWeight <= 0 {
resourceStrategyFitPluginWeight = DefaultResourceStrategyFitWeight
}
data, ok := cm.Data[DeviceConfigKey]
if !exist || nowTs-ts > TaskStatusUpdateIntervalSeconds {
Apply this practice to:
This makes the code self-documenting and centralizes configuration values for easier maintenance.
When documenting observability features such as metrics, debugging capabilities, or monitoring tools, always include clear information about their prerequisites, enabling conditions, and limitations. This ensures developers understand when these features are available and how to access them.
For metrics, specify any configuration flags or settings required:
| `argocd_github_api_requests_total` | counter | Number of Github API calls. |
> **Note**: All `argocd_github_api` metrics are only enabled when the corresponding feature flag is configured.
For debugging features, document the conditions that affect their availability:
## Debugging Options
- **IDE debugging**: Requires running components separately with proper build configuration
- **Process attachment**: Works with normally built binaries but not with `go run` processes (no debug symbols)
This practice prevents confusion and helps developers choose the appropriate observability approach for their specific setup and requirements.
When working with protocol buffer messages or similar objects that provide GetX() accessor methods, avoid redundant nil checks before calling these methods. GetX() methods are designed to handle nil receivers safely, allowing for cleaner and more concise code.
Good practice:
// These GetX() methods handle nil receivers safely
userData.GetData().GetPerType()[int32(taskQueueType)]
for _, v := range typedUserData.GetDeploymentData().GetVersions() {
// Process versions safely even if typedUserData is nil
}
Avoid unnecessary checks:
// Redundant and verbose - these checks are unnecessary
if userData != nil {
if userData.GetData() != nil {
typedUserData := userData.GetData().GetPerType()[int32(pm.Partition().TaskType())]
// ...
}
}
Important caveats:
Remember that while method chaining with GetX() is safe for retrieval operations, operations that modify values may still require explicit nil checks.
When defining protocol buffer messages, prioritize good organization and reuse:
Reuse existing message types instead of duplicating structures. This reduces maintenance effort and prevents potential bugs from diverging implementations.
Group related fields together and maintain consistent field ordering throughout your protocol definitions. When fields are conceptually related (like pass and id), they should always appear together and in the same order.
Example:
// GOOD: Reusing existing message types
message TaskQueueUserData {
api.temporal.server.v1.RateLimit rate_limit = 1;
}
// GOOD: Related fields grouped together in consistent order
message GetTaskQueueTasksRequest {
int64 min_pass = 5; // Related fields grouped together
int64 min_task_id = 6;
// Other fields...
}
Eliminate superfluous syntax and words that don’t add value to improve code readability and maintain consistent style. This includes removing unnecessary quotes in YAML files and avoiding redundant words in documentation.
For YAML files, avoid unnecessary quotes around simple string values. When quotes are required (e.g., for strings with special characters), prefer single quotes over double quotes:
# Good
type: git
url: https://example.com
name: 'special-name-with-hyphens'
# Avoid
type: "git"
url: "https://example.com"
In documentation and comments, remove redundant words that don’t contribute to clarity:
# Good
Finally, after the Linter reports no errors, run git status
# Avoid
Finally, after the Linter reports no errors anymore, run git status
This practice reduces visual clutter, improves readability, and maintains consistent formatting standards across the codebase.
Ensure consistency throughout the codebase by following established patterns for both UI components and code structure. Specifically:
// Bad
<Button _hover={{ bg: "gray.800" }} bg="black" color="white" />
// Good
<Button _hover={{ bg: "bg.emphasized" }} bg="bg.panel" color="fg.default" />
// When adding new toggle functionality, reuse icons from similar components
// Use the same expand/collapse icons as in ToggleGroups.tsx
// Bad - Repeating format strings
dayjs(date).tz(selectedTimezone).format("YYYY-MM-DD HH:mm:ss.SSS")
dayjs(anotherDate).tz(selectedTimezone).format("YYYY-MM-DD HH:mm:ss.SSS")
// Good - Define a constant
const DATE_TIME_FORMAT = "YYYY-MM-DD HH:mm:ss.SSS";
dayjs(date).tz(selectedTimezone).format(DATE_TIME_FORMAT)
dayjs(anotherDate).tz(selectedTimezone).format(DATE_TIME_FORMAT)
// Better - Extract a utility function for complex patterns
// In datetimeUtils.ts
export const formatDateTime = (date) => dayjs(date).tz(selectedTimezone).format(DATE_TIME_FORMAT);
By following these practices, you’ll create a more maintainable codebase where developers can easily understand and extend existing patterns.
Always use the configuration system’s default values instead of hardcoding defaults in the code. This ensures consistency across the application and makes configuration management more maintainable.
Bad:
DEFAULT_QUEUE: str = conf.get("operators", "default_queue", "default")
Good:
DEFAULT_QUEUE: str = conf.get_mandatory_value("operators", "default_queue")
For provider-specific configurations:
This approach centralizes configuration management, improves maintainability, and ensures consistent behavior across the application.
Always verify that pointers and nested struct fields are not nil before accessing their members to prevent runtime panics. This is especially important when dealing with optional configuration fields, middleware components, and API responses where certain fields may not be initialized.
Before accessing nested fields, check each level for nil:
// Bad - can cause nil pointer panic
*spec.SyncPolicy.Automated.Enable = true
// Good - check each level
if spec.SyncPolicy == nil {
spec.SyncPolicy = &argoappv1.SyncPolicy{}
}
if spec.SyncPolicy.Automated == nil {
spec.SyncPolicy.Automated = &argoappv1.SyncPolicyAutomated{}
}
*spec.SyncPolicy.Automated.Enable = true
For optional fields that may be missing, return nil gracefully rather than panicking:
// Good - handle missing claims safely
if _, ok := m["iat"]; !ok {
return nil, nil // Claim is missing
}
Consider creating helper methods on structs to encapsulate nil checks, making the code more readable and preventing repeated nil checking logic throughout the codebase.
Replace hardcoded strings, numbers, and magic values with well-named constants that clearly describe their purpose and meaning. This improves code readability, maintainability, and reduces the risk of typos when the same values are used in multiple places.
For string literals that are reused across the codebase, extract them into named constants:
// Instead of:
if (info.name === 'Requests (CPU)') {
// ...
}
if (info.name === 'Requests (MEM)') {
// ...
}
// Use:
const RESOURCE_NAMES = {
CPU_REQUESTS: 'Requests (CPU)',
MEMORY_REQUESTS: 'Requests (MEM)'
} as const;
if (info.name === RESOURCE_NAMES.CPU_REQUESTS) {
// ...
}
For magic numbers, use descriptive constants with explanatory comments when the logic isn’t immediately obvious:
// Instead of:
message += ' (' + revision.substring(0, 14) + ')';
// Use:
const SHA256_PREFIX_LENGTH = 8; // "sha256: " prefix
const DIGEST_DISPLAY_LENGTH = 7; // First 7 characters of actual digest
const TOTAL_DISPLAY_LENGTH = SHA256_PREFIX_LENGTH + DIGEST_DISPLAY_LENGTH;
message += ' (' + revision.substring(0, TOTAL_DISPLAY_LENGTH) + ')';
Additionally, ensure that variable and component names accurately reflect their actual functionality rather than misleading developers about their purpose.
Build SQLAlchemy queries that are both efficient and deterministic to prevent unpredictable results and improve performance. When writing database queries:
query.where((DagFavorite.dag_id == DagModel.dag_id) & (DagFavorite.user_id == self.user_id))
query.where(and_(DagFavorite.dag_id == DagModel.dag_id, DagFavorite.user_id == self.user_id))
2. **Build queries with explicit control flow** for complex conditions:
```python
# Instead of adding a complex boolean expression:
query = ...
since_running = Log.dttm > last_running_time if last_running_time else True
query = query.where(Log.event == EVENT, since_running)
# Prefer explicit conditional logic:
query = query.where(Log.event == EVENT)
if last_running_time:
query = query.where(Log.dttm > last_running_time)
query.order_by(AssetEvent.timestamp.asc())
query.order_by(AssetEvent.timestamp.asc(), AssetEvent.id.asc())
4. **Review query filters carefully** to avoid overly restrictive WHERE clauses that might unintentionally exclude valid records. Consider the complete range of valid states and values that should be included in your results.
---
## design extensible APIs
<!-- source: argoproj/argo-cd | topic: API | language: Go | updated: 2025-07-04 -->
When designing APIs, prioritize extensibility and backwards compatibility over convenience. Avoid modifying existing response types or interfaces in ways that could break existing clients. Instead, create new dedicated types, methods, or optional parameters.
Key principles:
- Create dedicated response objects instead of adding fields to existing types that could introduce breaking changes
- Add new interface methods or create separate interfaces rather than extending existing ones inappropriately
- Use optional parameters and configuration objects to support customization without breaking existing usage
- Consider the impact on existing clients when modifying API contracts
Examples:
```go
// Instead of modifying existing ApplicationList
type ApplicationList struct {
Items []Application `json:"items"`
Stats ApplicationListStats `json:"stats,omitempty"` // Breaking change!
}
// Create a dedicated response object
type ApplicationListResponse struct {
Applications ApplicationList `json:"applications"`
Stats ApplicationListStats `json:"stats,omitempty"`
}
// Instead of extending existing interface inappropriately
type Provider interface {
Verify(tokenString string) (*gooidc.IDToken, error)
VerifyJWT(tokenString string) (*jwtgo.Token, error) // Mixing concerns
}
// Create separate interfaces or new implementations
type JWTProvider interface {
VerifyJWT(tokenString string) (*jwtgo.Token, error)
}
// Support optional parameters for extensibility
func NewClient(repoURL string, creds Creds, opts ...ClientOpts) (Client, error) {
// Allows adding new options without breaking existing calls
}
This approach ensures APIs can evolve gracefully while maintaining compatibility with existing integrations.
Use consistent parameter handling patterns across API endpoints to ensure maintainability and predictable behavior. Follow these guidelines:
def get_dependencies( node_ids: list[str] | None = Query(None) ) -> BaseGraphResponse: pass
def get_dependencies( node_ids: str | None = Query(None, description=”Comma-separated list of node ids”) ) -> BaseGraphResponse: ids_to_fetch = [nid.strip() for nid in node_ids.split(“,”)] if node_ids else []
2. Define constants for query parameter limits:
```python
MAX_SORT_PARAMS = 2
def get_order_by_columns(self, order_by_list: list[str]) -> list:
if len(order_by_list) > MAX_SORT_PARAMS:
raise HTTPException(
400,
f"Ordering with more than {MAX_SORT_PARAMS} parameters is not allowed"
)
def list_items(
offset: int = 0,
limit: int = 50, # Make limits configurable
) -> Response:
pass
This standardization improves API usability, reduces code duplication, and makes the API behavior more predictable for clients.
Documentation should follow established best practices to ensure clarity, consistency, and usefulness:
Task SDK Overview <../concepts/taskflow.html>_Task SDK Overview <https://airflow.apache.org/docs/apache-airflow/stable/concepts/taskflow.html>_
```Write in complete sentences: Ensure all descriptions, especially in API documentation and migration notes, are complete sentences with proper grammar and punctuation.
@task
def process_data(data_path: str, limit: Optional[int] = None) -> Dict[str, Any]:
"""
Process data from the specified path with optional limit.
:param data_path: Path to the data file to process
:param limit: Maximum number of records to process, None for unlimited
:return: Dictionary containing processed results
"""
# Implementation
Document all API variants: Ensure “sub” decorators and related API components (like @task.skip_if) are documented alongside their parent components.
exampleinclude instead of literalinclude to provide “[sources]” links that give users access to complete context and code examples.Following these practices improves documentation usability, making it easier for developers to understand and correctly implement the documented functionality.
Clearly define and document the boundaries between public and internal APIs to guide developers on proper interface usage. Use dedicated namespaces (like the ‘sdk’ namespace) to distinguish public interfaces from internal implementations. Ensure all public interfaces have comprehensive documentation that accurately reflects behavior, with consistent terminology across all references.
When creating extension points for your API, document them thoroughly with examples:
# Example of a well-documented API extension point
class AirflowApiExtensionPlugin(AirflowPlugin):
"""Plugin that extends the Airflow API with custom endpoints.
This plugin allows adding FastAPI applications to extend Airflow's API.
"""
fastapi_apps = [
{
"app": app, # FastAPI app instance
"url_prefix": "/my-api-extension", # URL prefix for all endpoints
"name": "My API Extension", # Descriptive name shown in UI
}
]
For client interfaces, document all available methods, parameters, and expected responses. Include information about error handling, authentication requirements, and rate limiting where applicable. Explicitly state which components are guaranteed to remain stable across versions and which may change, helping developers make informed decisions about API dependencies.
Maintain consistent naming patterns across related components and services. When implementing equivalent functionality in different services, use identical field/method names. When naming related fields that form logical groupings, either:
For request/response objects:
UpdateTaskQueueConfig should match the equivalent in workflowservice)heartbeat_request instead of request)Example:
// GOOD: Consistent suffix pattern for related fields
int64 ack_level_pass = 4;
int64 ack_level_id = 2;
// BETTER: Using a structured type
message Level {
int64 pass = 1;
int64 id = 2;
}
Level ack_level = 2;
// BAD: Inconsistent naming pattern
int64 ack_level_pass = 4;
int64 ack_level = 2; // Should be ack_level_id
Consistent naming reduces cognitive load, improves readability, and helps prevent errors when working across service boundaries.
Implement strict authentication boundaries and access controls to prevent security vulnerabilities:
Example:
def authenticate(self):
if self.jwt_token and self.username and self.password:
raise AirflowException(
"Both JWT and Password/Username authentication provided. "
"Please configure only one authentication method."
)
if self.jwt_token:
return self._authenticate_with_jwt()
elif self.username and self.password:
return self._authenticate_with_credentials()
else:
raise AirflowException("No valid authentication method configured")
Restrict direct access to sensitive resources (like databases) by implementing authentication barriers and API-based access patterns. This follows the principle of least privilege, where components should only have the minimum access necessary to perform their functions.
For token-based authentication systems, implement proper token lifecycle management including secure issuance, validation, renewal, and expiration handling through well-defined communication channels.
Access configurations through documented abstraction layers rather than bypassing them with direct database or low-level access. For example, in Airflow, XCom operations should be performed through the Task Context using get_current_context() rather than directly accessing the database model:
# Good practice
from airflow.sdk import get_current_context
def my_task(**context):
# Alternatively: context = get_current_context()
# Pull XCom value
value = context["ti"].xcom_pull(task_ids="previous_task")
# Push XCom value
context["ti"].xcom_push(key="my_key", value="my_value")
For version-dependent configurations, use explicit version comparison functions (like semverCompare in Helm) rather than hard-coding version assumptions:
# Good practice (Helm example)
{{- if and (semverCompare "<3.0.0" .Values.airflowVersion) (or .Values.webserver.webserverConfig .Values.webserver.webserverConfigConfigMapName) }}
# Version-specific configuration here
{{- end }}
Extract repeated code patterns into reusable functions, variables, or constants to improve maintainability and reduce the risk of inconsistencies. Look for similar code blocks that perform the same operations and consolidate them.
For repeated code blocks:
# Instead of duplicated validation code:
def _validate_inputs(self):
"""Validate required inputs."""
missing_fields = []
for field_name in ["sink_name", "project_id"]:
if not getattr(self, field_name):
missing_fields.append(field_name)
if missing_fields:
raise AirflowException(
f"Required parameters are missing: {missing_fields}. These parameters must be passed as "
"keyword parameters or as extra fields in Airflow connection definition."
)
# Prefer a shared validation function:
def validate_required_fields(obj, fields):
"""Validate that required fields are present."""
missing_fields = []
for field_name in fields:
if not getattr(obj, field_name):
missing_fields.append(field_name)
if missing_fields:
raise AirflowException(
f"Required parameters are missing: {missing_fields}. These parameters must be passed as "
"keyword parameters or as extra fields in Airflow connection definition."
)
For repeated values:
# Instead of:
command = ["sudo", "-E", "-H", "-u", run_as_user, sys.executable, "-c", rexec_python_code]
log.info("Running command", command=command)
os.execvp("sudo", command)
# Store in a variable to avoid duplication:
command = ["sudo", "-E", "-H", "-u", run_as_user, sys.executable, "-c", rexec_python_code]
log.info("Running command", command=command)
os.execvp("sudo", command)
For repeated logic:
# Instead of repeated conditionals:
if len(order_by_list) >= 1:
order_by_columns.append(self.get_column_with_sort(order_by_list[0]))
if len(order_by_list) == 2:
order_by_columns.append(self.get_column_with_sort(order_by_list[1]))
# Use a loop:
for i in range(min(len(order_by_list), MAX_SORT_PARAMS)):
order_by_columns.append(self.get_column_with_sort(order_by_list[i]))
Always check for null values before using them, and use Python’s idiomatic patterns to make these checks concise and readable. This prevents unexpected errors and makes code more maintainable.
There are several approaches to handle null values effectively:
# Instead of:
url = cast("str", conn.schema) + "://" + cast("str", conn.host)
# Do this:
if conn.schema is None:
raise AirflowException("Schema field is missing and is required.")
if conn.host is None:
raise AirflowException("Host field is missing and is required.")
url = conn.schema + "://" + conn.host
# Instead of:
processor = self._processors.pop(proc, None)
if processor:
processor.logger_filehandle.close()
# Use:
if processor := self._processors.pop(proc, None):
processor.logger_filehandle.close()
# Instead of:
dag_version = DagVersion.get_latest_version(dag.dag_id, session=session)
if TYPE_CHECKING:
assert dag_version
# Do proper null checking:
dag_version = DagVersion.get_latest_version(dag.dag_id, session=session)
if dag_version is None:
raise ValueError(f"No version found for DAG {dag.dag_id}")
Proper null handling improves code readability, prevents common runtime errors, and makes intentions clear to both humans and static analysis tools.
Always prioritize reusing existing components before creating new ones. This practice improves codebase consistency, reduces maintenance burden, and prevents duplication. When implementing new UI elements:
Example - Bad:
// Creating a new input component when a SearchBar already exists
<InputGroup startElement={<FiHash size={14} />}>
<Input
maxW="200px"
onChange={handleRunIdChange}
placeholder={translate("dags:filters.runIdFilter")}
value={filteredRunId ?? ""}
/>
</InputGroup>
Example - Good:
// Reusing the existing SearchBar component
<SearchBar
onChange={handleRunIdChange}
placeholder={translate("dags:filters.runIdFilter")}
value={filteredRunId ?? ""}
/>
Example - Bad:
// Custom SVG implementation
const ChevronDownIcon = () => (
<svg fill="currentColor" height="1em" viewBox="0 0 20 20" width="1em">
<path
clipRule="evenodd"
d="M5.23 7.21a.75.75 0 011.06.02L10 11.085l3.71-3.855a.75.75 0 111.08 1.04l-4.24 4.4a.75.75 0 01-1.08 0l-4.24-4.4a.75.75 0 01.02-1.06z"
fillRule="evenodd"
/>
</svg>
);
Example - Good:
// Using existing icon library
import { FiChevronDown } from "react-icons/fi";
// Then in your component
<FiChevronDown />
Additionally, maintain framework consistency by using framework-specific patterns (like Chakra UI factories) rather than mixing with HTML elements:
// Instead of <details>...</details>
<chakra.details open={open} w="100%">
{/* content */}
</chakra.details>
This approach ensures better styling consistency, accessibility support, and reduces the need to reinvent existing functionality.
Use proper naming conventions to make code more readable and maintainable:
// Incorrect
const hasChanges = () => { ... }
// Correct
const checkForChanges = () => { ... }
// Too generic
element.download = `taskInstanceLogs.txt`;
// Better - includes relevant identifiers
element.download = `${dagId}-${taskId}-${runId}-${mapIndex}-${tryNumber}.txt`;
// Define consistent casing pattern for categories
const existingCategories = ["user", "docs", "admin", "browse"];
// Use consistent transformation when comparing
if (!existingCategories.includes(category.toLowerCase())) {
// ...
}
Following these conventions improves code clarity and reduces confusion when maintaining code across a team.
When documenting configuration options, provide comprehensive information including current default values, recommended settings, future behavioral changes, and potential security implications. Configuration comments should clearly explain not just what the option does, but also warn about risks and provide guidance on proper usage.
For example, instead of a simple description:
# Set this flag to false to revert to old behavior
application.sync.externalRevisionConsideredOverride: "true"
Provide comprehensive documentation:
# The current behavior allows passing a different revision from the one given in the application when syncing. We highly recommend that this be set to `true`. The next major release will set the default to be `false`.
application.sync.externalRevisionConsideredOverride: "false"
# When set to true, the ApplicationSet controller will continue to generate Applications even if the repository is not found.
# NOTE: If a repository exists but is inaccessible due to access rights, SCM providers usually return a "404 Not Found" error
# instead of a "403 Permission Denied" error. Consequently, using this option may lead to the deletion of Argo CD applications
# if the SCM user associated with the token loses access to the repository.
This prevents misconfigurations that could lead to security vulnerabilities or unexpected application behavior.
Ensure log messages provide clear context about what happened, potential consequences, and actionable information for debugging. Choose appropriate log levels based on severity, and include exception details when catching errors.
Good log messages significantly improve troubleshooting efficiency by providing developers with information needed to understand issues without requiring code examination.
Examples:
# Instead of vague messages:
log.warning("Failed to get user name from os: %s", e)
# Provide context about impact:
log.warning("Failed to get user name from os: %s, not setting the triggering user", e)
# When handling exceptions, log the full exception:
try:
sqs_get_queue_attrs_response = self.sqs_client.get_queue_attributes(
QueueUrl=queue_url, AttributeNames=["ApproximateNumberOfMessages"]
)
# Process response...
except Exception as e:
# Include details for better debugging
log.error("SQS connection health check failed: %s", e)
Consider what information would be most helpful to developers trying to diagnose an issue from logs alone.
Always prefer explicit configuration parameters over inferring behavior from indirect sources like image tags, naming conventions, or environment factors. When behavior needs to vary based on environment or version, introduce dedicated configuration parameters that users can explicitly set rather than relying on potentially unreliable metadata.
This practice ensures that:
For example, instead of:
# DON'T: Inferring behavior from image tag
{{- if hasPrefix .Values.images.gitSync.tag "v3" }}
# Use v3 configuration
{{- else }}
# Use v4 configuration
{{- end }}
Use:
# DO: Explicit configuration parameter
gitSync:
version: "v4" # Explicit parameter users can set
{{- if eq .Values.gitSync.version "v3" }}
# Use v3 configuration
{{- else }}
# Use v4 configuration
{{- end }}
When adding configuration parameters that might interact with existing ones, clearly document their relationships and ensure proper handling of all cases. For instance, document when certain parameters might be ignored under specific conditions, and consider providing mechanisms to merge or append user-defined values with defaults.
Write parameterized tests instead of using for loops or duplicating test code for similar test cases. Using pytest’s @pytest.mark.parametrize decorator improves test maintainability, provides clearer failure messages, and makes the test structure more readable.
Instead of:
def test__read_for_executor_fallbacks(self, create_task_instance):
for state in (TaskInstanceState.RUNNING, TaskInstanceState.DEFERRED, TaskInstanceState.UP_FOR_RETRY):
ti = create_task_instance(dag_id="test_dag", task_id="test_task")
ti.state = state
# Test implementation with same logic for each state
...
Use:
@pytest.mark.parametrize(
"state",
[TaskInstanceState.RUNNING, TaskInstanceState.DEFERRED, TaskInstanceState.UP_FOR_RETRY]
)
def test__read_for_executor_fallbacks(self, state, create_task_instance):
ti = create_task_instance(dag_id="test_dag", task_id="test_task")
ti.state = state
# Test implementation
...
This approach applies to other types of test parameterization as well, such as organizing related test cases together in parameterized test functions rather than duplicating test logic with slight variations. For endpoint testing, organize tests with one class per endpoint and multiple parameterized test methods for different test cases.
Add explanatory comments for non-obvious code decisions that might appear incorrect, unusual, or suboptimal to future contributors. Without such comments, code that looks unusual might be “fixed” during refactoring, introducing bugs. This is particularly important for:
For example:
# Deliberately returning 403 without details to avoid information leakage
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Invalid token")
# Using _get_response() only for initial message where parent sends unsolicited data
msg = comms_decoder._get_response()
# Explicit attribute definition to prevent automatic detection which would be error-prone
dependent_tables: list[str] | None = None
These comments explain the “why” behind code decisions that might not be immediately obvious, preventing well-intentioned changes that could break intended behavior.
Prefer explicit type guards and null checks over TypeScript’s type assertions (as) when handling potentially null or undefined values. Type assertions bypass TypeScript’s type checking system and can lead to runtime errors.
Instead:
??) to provide default valuesArray.isArray() and property checksExample (Before):
const menuPlugins = (useConfig("plugins_extra_menu_items") as Array<ExternalViewResponse>) ?? [];
Example (After):
const menuItems = useConfig("plugins_extra_menu_items");
const menuPlugins = Array.isArray(menuItems) ? menuItems : [];
// Or with additional type validation:
if (
Array.isArray(menuItems) &&
menuItems.every(item => "name" in item && "href" in item)
) {
// Safe to use menuItems here
}
This approach ensures runtime safety beyond compile-time type checking and reduces the risk of unexpected null reference exceptions.
When switching between different configuration sources (like APIs or configuration values), ensure data structure compatibility and consistent behavior. Configuration endpoints may return different data structures that require proper transformation.
For example, instead of this direct replacement:
- const { data } = usePluginServiceGetPlugins();
+ const { data } = useConfig("plugins_extra_menu_items");
Ensure proper data transformation:
- const { data } = usePluginServiceGetPlugins();
+ const menuPluginsData = useConfig("plugins_extra_menu_items");
+ const menuPlugins = menuPluginsData ? transformConfigToPluginFormat(menuPluginsData) : [];
Always test your changes with multiple configuration scenarios, and consider backward compatibility when configuration structures are in transition. Verify UI behavior remains consistent across all possible configuration states.
Always validate and sanitize user-provided inputs used in file path operations to prevent path traversal attacks. Path traversal vulnerabilities can allow attackers to access unauthorized files outside the intended directory structure.
Key implementation steps:
Example implementation:
def _validate_log_file_path(filename: str, log_directory: str) -> Path:
"""
Validate that the requested file path is within the log directory and safe to serve.
"""
# URL decode the filename to handle encoded path traversal attempts
try:
decoded_filename = urllib.parse.unquote(filename)
except Exception as e:
logger.warning("Failed to URL decode filename '%s': %s", filename, e)
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Invalid URL encoding in filename",
)
# Check for control characters and null bytes
if not decoded_filename or any(ord(c) < 32 for c in decoded_filename):
logger.warning("Invalid characters detected in filename")
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Invalid characters in filename",
)
if "\x00" in decoded_filename:
logger.warning("Null byte detected in filename")
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Null byte in filename",
)
# Normalize paths and verify they remain within allowed directory
log_dir = Path(log_directory).resolve()
file_path = log_dir / decoded_filename
resolved_path = file_path.resolve()
if not resolved_path.is_relative_to(log_dir):
logger.warning("Path traversal attempt detected")
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="Access denied: path outside log directory",
)
# Verify file type (not a directory or special file)
if resolved_path.exists() and not resolved_path.is_file():
logger.warning("Attempt to access non-file resource")
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail="Access denied: not a regular file",
)
return resolved_path
For security best practices, avoid logging the actual path contents in error messages to clients. Only log detailed error information server-side to avoid providing attackers with information that could help refine their attacks.
Use specific exception types and avoid masking errors through overly broad exception handling or default values. This improves error diagnosis and system reliability.
Key guidelines:
Example - Instead of:
try:
result = client.refresh_token(token)
except Exception as e:
log.error("Error refreshing token: %s", e)
return None
Use:
def refresh_token(self, refresh_token: str) -> str:
"""Refresh the access token for the user."""
client = self._get_keycloak_client()
tokens = client.refresh_token(refresh_token)
log.info("Token refreshed successfully")
return tokens["access_token"]
This approach:
Use structured logging with proper context and consolidate related log messages. Instead of multiple separate log statements or string interpolation, use structured fields that are parseable by log tools and provide sufficient context for debugging.
Key practices:
Example:
// Instead of:
log.Warnf("Failed to verify token: %s", err)
log.Infof("Client IP: %s", r.RemoteAddr)
// Use:
log.WithFields(log.Fields{
"client_ip": r.RemoteAddr,
"error": err,
}).Warn("Failed to verify token")
// For application logs, use standard fields:
logCtx.WithField("application", appName).Errorf("validation error: %s", message)
This approach makes logs more searchable, reduces noise, and provides better context for troubleshooting.
Apply concurrency control mechanisms with precise scope to prevent both race conditions and unnecessary blocking. Use appropriate synchronization tools (mutex, channels, errgroup) only around critical sections that actually require protection.
Common patterns to follow:
// Bad - overly broad mutex scope mux.Lock() defer mux.Unlock() // Blocking concurrent operations unnecessarily runProviders()
2. Use errgroup for concurrent error handling:
```go
eg := errgroup.Group{}
for _, container := range containers {
container := container // Capture loop variable
eg.Go(func() error {
return processContainer(ctx, container)
})
}
return eg.Wait()
// Good - protected access var mu sync.Mutex for _, svc := range services { svc := svc go func() { mu.Lock() opts[svc.Name] = buildOptions() mu.Unlock() }() }
---
## Prevent sensitive data exposure
<!-- source: docker/compose | topic: Security | language: Go | updated: 2025-06-30 -->
Implement comprehensive checks to prevent accidental exposure of sensitive data through multiple vectors in configuration files and deployment artifacts. This includes validating environment variables, file permissions, bind mounts, and individual configuration files before publishing or deployment.
Key practices:
1. **Environment variables**: Block publication of compose files containing environment variables or env_files by default, requiring explicit opt-in flags to prevent accidental secret leaks
2. **Individual file validation**: Check each individual file in compose projects for sensitive data, not just the final merged model, as secrets may be present in individual files even if overridden
3. **File permissions**: Use restrictive permissions for sensitive files (e.g., 0o440 for secrets vs 0o444 for regular configs) to limit access
4. **User warnings**: Provide clear warnings when potentially sensitive data like bind mount declarations will be included in artifacts
Example implementation:
```go
func preChecks(project *types.Project, options api.PublishOptions) error {
if !options.WithEnvironment {
for _, service := range project.Services {
if len(service.Environment) > 0 {
return fmt.Errorf("service %q has environment variable(s) declared. To avoid leaking sensitive data, " +
"you must either explicitly allow the sending of environment variables by using the --with-env flag")
}
}
}
// Check individual files, not just final model
// Validate bind mounts with user warnings
// Apply restrictive permissions for secrets
}
This approach ensures multiple layers of protection against sensitive data exposure while maintaining usability through explicit opt-in mechanisms.
Names should clearly indicate purpose and behavior using appropriate action verbs and descriptive terms. This applies to functions, methods, variables, and classes. Avoid generic or ambiguous names that don’t convey the actual functionality.
Key guidelines:
Example of good naming:
# Better names that clearly indicate purpose
def sync_to_local_dir(self, bucket_name: str): # vs generic "download_s3"
def add_debug_span(func): # vs ambiguous "add_span"
# Consistent naming pattern for related properties
class ViewResponse(BaseModel):
icon: str | None = None
icon_dark_mode: str | None = None # vs disconnected "dark_mode_icon"
# Avoid shadowing in loops
for progress_line in progress_callback_lines: # vs reusing "line"
process(progress_line)
All user interface text must be internationalized using the appropriate i18n module instead of hardcoded strings. This ensures the application can be properly localized for different languages and regions.
Key implementation guidelines:
useTranslation hook to access translation stringsen/common.json, en/dashboard.json)Example:
// Incorrect approach
<Heading ml={1} size="xs">
Favorite Dags
</Heading>
// Correct approach
import { useTranslation } from "react-i18next";
// Within component
const { t: translate } = useTranslation("dashboard");
<Heading ml={1} size="xs">
{translate("favorite.favoriteDags")}
</Heading>
When adding new UI text:
This practice improves accessibility, enables global adoption, and follows proper documentation standards for international software.
When localizing an application, maintain consistent conventions for technical terms while respecting language-specific rules:
// English
"dag_one": "Dag",
"dagRun_one": "Dag Run",
// Spanish
"dag_one": "DAG",
"dagRun_one": "Ejecución del DAG"
This approach ensures that the UI remains intuitive for developers working in different languages while respecting established conventions in each target language.
When providing translations for user interface elements and documentation, prioritize natural and idiomatic expressions in the target language rather than literal translations. Consider the grammatical rules and cultural context of each language to ensure the translated text sounds native to users.
Follow these best practices:
Example:
// Instead of literal translation
"wrap": {
"tooltip": "Pressione {{hotkey}} para alternar o wrap",
"unwrap": "Desembrulhar",
"wrap": "Embrulhar"
}
// Use natural, context-appropriate translation
"wrap": {
"tooltip": "Pressione {{hotkey}} para expandir ou recolher a secção",
"unwrap": "Expandir",
"wrap": "Recolher"
}
This approach makes the documentation and UI more intuitive and accessible to international users, improving the overall user experience across different language contexts.
When designing CI/CD workflows, prioritize both resource efficiency and clear documentation. Incorporate dynamic configuration where beneficial, but avoid creating separate jobs for simple tasks that can be handled within existing steps. Ensure all comments and configurations accurately reflect version requirements across all supported branches, particularly when backporting is necessary.
Example for efficient runner configuration:
build-info:
name: "Build info"
runs-on: >-
# Dynamic runner configuration that doesn't require a separate job
# Explanation of why this approach was chosen and what it accomplishes
Maintain clear documentation about version requirements in configuration files, especially when different branches have different version support. When a comment mentions supported versions (like Python versions), ensure it’s accurate and consider adding notes about special cases like backporting requirements.
Always specify explicit version constraints for dependencies in configuration files (like pyproject.toml), particularly when dealing with Python version compatibility. Use conditional dependencies based on Python versions to handle compatibility issues, and include explanatory comments when adding constraints.
For example:
# Python 3 saml is not compatible with Python 3.13 yet, so we pin it
"python3-saml>=1.16.0; python_version < \"3.13\"",
# Specify different versions for different Python versions
"pandas>=2.1.2; python_version < \"3.13\"",
"pandas>=2.3.0; python_version >= \"3.13\"",
This practice prevents build failures due to incomplete package metadata, especially when working with lowest compatible versions or when dependencies don’t proactively specify Python version exclusions. Use an optimistic approach for upper bounds unless tests specifically indicate incompatibility.
When disabling security-related linter rules or bypassing security best practices, always include comments that explain why the exception is necessary, why it remains secure, and link to relevant security policies. This creates an audit trail and ensures future developers understand the security implications.
Example:
/* eslint-disable react/iframe-missing-sandbox */
// Exception justified: This iframe only loads content from our AuthManager
// which is part of the deployment and considered trusted per our security policy:
// https://airflow.apache.org/docs/apache-airflow/stable/security/security_model.html
<iframe src={trustedSource} title="Auth content" />
Avoid performing expensive operations repeatedly, especially in hot code paths. Identify operations such as parsing, deserialization, or configuration lookups that can be done once and reused.
Three key strategies:
// Optimized: Parse once during initialization func newWorkerQueryEngine(nsID string, query string) *workerQueryEngine { parsedQuery, _ := prepareQuery(query) // Parse once return &workerQueryEngine{ nsID: nsID, query: query, parsedQuery: parsedQuery, } }
2. **Cache expensive results**:
```go
// Add a TODO for caching deserialized tasks
taskValue, err := deserializeTask(registrableTask, taskInfo.Data)
// TODO: Cache the deserialized task to avoid repeated deserialization
// Efficient: Returns pointer to existing data return &cvs[idx], nil // Returns pointer into slice, avoiding allocation
4. **Calculate values once and pass as parameters**:
```go
// Inefficient: Repeatedly evaluating expensive config
for _, start := range starts {
if e.shouldYield(scheduler, *actionsTaken) { // Config lookup each time
break
}
// ...
}
// Efficient: Calculate once and reuse
tweakables := e.Config.Tweakables(scheduler.Namespace)
maxActions := tweakables.MaxActionsPerExecution
for _, start := range starts {
if *actionsTaken >= maxActions {
break
}
// ...
}
Also consider proper ordering of operations - perform cheap checks before expensive ones. For example, check exclusion conditions before making dynamic config calls.
Documentation should provide thorough technical explanations that define key concepts, explain behaviors, and include necessary context for users to properly understand and use features. Avoid leaving users to guess what terms mean or how features differ from similar functionality.
Key practices:
Example of insufficient explanation:
--compare-desired Compare revision with desired state instead of live state
Example of comprehensive explanation:
--compare-desired Compare revision with desired state instead of live state
"Desired state" refers to the configuration stored in Git that Argo CD should apply.
This differs from the normal diff behavior which compares:
- Live state (current cluster state) vs desired state (Git configuration)
Instead, this flag compares:
- Desired state (Git configuration) vs desired state (Git configuration at different revision)
Always quote shell script variable expansions using double quotes to prevent word splitting issues and ensure consistent behavior. This is particularly important in conditionals and when passing variables as command arguments.
Bad:
rm -rf /usr/local/lib/python${PYTHON_MAJOR_MINOR_VERSION}/site-packages/
if [[ ${UPGRADE_SQLALCHEMY=} != "true" ]]; then
Good:
rm -rf /usr/local/lib/python"${PYTHON_MAJOR_MINOR_VERSION}"/site-packages/
if [[ "${UPGRADE_SQLALCHEMY}" != "true" ]]; then
Unquoted variables can lead to unexpected behavior when they contain spaces, are empty, or include special characters. Consistently applying proper quoting improves script robustness and prevents subtle bugs that might only appear in edge cases.
When implementing network topology-aware scheduling, ensure that the appropriate topology plugins are properly configured for different scheduling modes. In hard mode, the scheduler should strictly enforce topology constraints, while in soft mode, it should prefer network locality but allow cross-topology scheduling when necessary.
For hard mode scheduling, verify that jobs are constrained to single hypernodes or tiers as specified:
job := &e2eutil.JobSpec{
Name: "topology-job",
NetworkTopology: &batchv1alpha1.NetworkTopologySpec{
Mode: batchv1alpha1.HardNetworkTopologyMode,
HighestTierAllowed: ptr.To(1),
},
// ... task specifications
}
For soft mode testing, enable the network-topology-aware plugin in the scheduler configuration to ensure pods within the same job are co-located when possible:
# volcano-scheduler-ci.conf
plugins:
- name: network-topology-aware
enabledHierarchy: "*"
When designing test scenarios, ensure resource requests and cluster topology accurately reflect the intended scheduling behavior. For multi-tier scenarios, configure sufficient resource pressure to force scheduling across tiers while maintaining network locality preferences. This prevents tests from passing due to unrelated factors like bin-packing algorithms rather than proper topology awareness.
Use Python’s built-in efficient collection processing methods instead of manual implementations. This reduces code complexity and improves performance by leveraging optimized implementations.
Key practices:
Example transformations:
# Instead of multiple set operations:
t_dags = {task.dag for task in tasks if not isinstance(task, tuple)}
t_dags_2 = {item[0].dag for item in tasks if isinstance(item, tuple)}
task_dags = t_dags | t_dags_2
# Use list comprehension:
task_dags = {
task[0].dag if isinstance(task, tuple) else task.dag
for task in tasks
}
# Instead of sorting and indexing:
items.sort(key=lambda x: x.end_date, reverse=True)
last_item = items[0] if items else None
# Use max():
last_item = max(items, key=lambda x: x.end_date) if items else None
# Instead of multiple isinstance checks:
if isinstance(log, chain) or isinstance(log, GeneratorType):
...
# Use tuple of types:
if isinstance(log, (chain, GeneratorType)):
...
When introducing new identifiers (variables, methods, flags, metrics, etc.), examine existing code to identify and follow established naming conventions rather than creating new patterns. This ensures consistency across the codebase and maintains predictability for users and developers.
Before naming new elements, look for similar existing components and adopt their naming patterns. For example, when adding CLI flags, follow the pattern of sibling commands using long descriptive names like --app-namespace and --hard-refresh. For metrics, follow established conventions like appending _total to counter metrics (e.g., argocd_login_request_total instead of argocd_login_request).
This approach prevents naming inconsistencies that can confuse users and makes the codebase more maintainable by establishing predictable patterns that developers can easily follow.
Configure OpenTelemetry programmatically rather than through environment variables or command-line flags. This creates a more maintainable and standardized approach to observability. Additionally, ensure all observability components like collectors are explicitly included in your deployment configurations.
Example:
# Preferred - Adding collector to Docker Compose services
services: "${{ format('{0}\notel-collector', join(matrix.containers, '\n')) }}"
# Avoid passing as environment variables
# make OTEL=true functional-test-coverage # Not recommended
This approach ensures consistent observability setup across environments and makes configuration changes more traceable through source control.
When consuming APIs in frontend applications, utilize the backend capabilities rather than reimplementing equivalent functionality in the frontend. This reduces code complexity, improves performance, and maintains a clean separation of concerns.
Key practices:
// PREFER const { data } = useAssetServiceGetAssets({ limit: MAX_VISIBLE, offset: 0 });
2. **Request appropriate content formats**: Use Accept headers to get data in the format best suited for your needs.
```typescript
// When raw logs are needed
useTaskInstanceServiceGetLog({
// Other parameters...
accept: "text/plain" // Get raw logs directly
});
// PREFER - Extend the backend to support this use case const { data } = useAssetServiceGetAssets({ search: searchValue }); // Searches across multiple fields
4. **Extend the backend when needed**: If you need functionality that isn't available in the API, consider extending the backend API rather than creating complex workarounds in the frontend.
Following these practices leads to more maintainable code, better performance, and a clearer separation of responsibilities between frontend and backend.
---
## Explicit CI configuration conditions
<!-- source: temporalio/temporal | topic: CI/CD | language: Yaml | updated: 2025-06-23 -->
When writing CI/CD workflow configurations, always use explicit and precise conditions, paths, and selectors to ensure that actions only execute when necessary and operate on the exact intended targets. This prevents wasted compute resources, unintended side effects, and improves workflow reliability.
For file paths in artifact uploads or processing steps, specify exact patterns instead of entire directories:
```yaml
# Instead of this (too broad):
path: .testoutput
# Use this (precise):
path: ./.testoutput/junit.*.xml
For conditional execution, include all necessary checks to ensure steps only run when appropriate:
# Instead of this (missing check):
if: ${{ inputs.run_single_functional_test != true || (inputs.run_single_functional_test == true && contains(fromJSON(needs.set-up-single-test.outputs.dbs), env.PERSISTENCE_DRIVER)) }}
# Use this (complete check):
if: ${{ toJson(matrix.containers) != '[]' && (inputs.run_single_functional_test != true || (inputs.run_single_functional_test == true && contains(fromJSON(needs.set-up-single-test.outputs.dbs), env.PERSISTENCE_DRIVER))) }}
This practice reduces CI resource usage, prevents accidental inclusion of unwanted files in artifacts, and makes debugging workflow issues significantly easier.
When implementing authentication token handling, prioritize more secure storage mechanisms over less secure ones. Specifically, prefer retrieving tokens from properly configured cookies (with Secure and HttpOnly flags) before falling back to localStorage, which is vulnerable to XSS attacks.
Why: Cookies with proper security attributes (HttpOnly, Secure, SameSite) provide better protection against common web vulnerabilities like XSS attacks compared to localStorage or sessionStorage, which can be accessed by any JavaScript running on your page.
Example:
// Insecure approach: localStorage first, cookies as fallback
const token = localStorage.getItem(TOKEN_STORAGE_KEY) ?? getTokenFromCookies();
// Secure approach: properly configured cookies first, localStorage as fallback
const token = getTokenFromCookies() ?? localStorage.getItem(TOKEN_STORAGE_KEY);
When using cookies for token storage, ensure they are configured with appropriate security flags (HttpOnly, Secure, SameSite) to maximize protection against common web attacks.
Always propagate context through network calls to ensure proper cancellation, timeout handling, and tracing. For HTTP requests, use http.NewRequestWithContext() instead of http.Post() or http.Get(). For gRPC, ensure context is passed and enriched with necessary metadata.
When adding metadata to context:
metadata.AppendToOutgoingContext(ctx, key, value)metadata.AppendToOutgoingContext(ctx, k1, v1, k2, v2, ...)Example of context-aware HTTP client:
// Instead of this:
response, err := http.Post(a.opaEndpoint, "application/json", bytes.NewBuffer(jsonData))
// Do this:
req, err := http.NewRequestWithContext(ctx, "POST", a.opaEndpoint, bytes.NewBuffer(jsonData))
if err != nil {
return resultDeny, err
}
req.Header.Set("Content-Type", "application/json")
response, err := http.DefaultClient.Do(req)
Example of proper gRPC context metadata:
// Batch metadata in a single call
ctx = metadata.AppendToOutgoingContext(ctx,
"temporal-test-name", testName,
"user-id", userId,
"request-id", requestId)
Context propagation ensures that cancellation signals are properly passed throughout the system, preventing resource leaks and allowing proper tracing of distributed operations.
Always use structured logging with appropriate tags instead of string concatenation or formatting. Include relevant contextual information that would help in troubleshooting, such as namespace, operation ID, or workflow ID.
Instead of this:
logger.Debug("ShowTaskQueueConfig : " + strconv.FormatBool(req.ShowTaskQueueConfig))
logger.Info(fmt.Sprintf("Removed expired workflow rule %s", oldestKey))
Do this:
logger.Debug("Show task queue config", tag.Bool("showTaskQueueConfig", req.ShowTaskQueueConfig))
logger.Info("Removed expired workflow rule", tag.WorkflowRuleID(oldestKey), tag.WorkflowNamespaceID(namespaceID))
Structured logging with proper context tags makes logs:
Always consider what contextual information would be helpful for someone debugging an issue, and include those as tags rather than embedding them in the message text.
When working with CSS/SCSS, prioritize organization and avoid duplication by following these principles:
Extract related styles into dedicated files - Group thematically related CSS rules (like motion controls, accessibility overrides, or component variants) into separate SCSS files and import them centrally for better maintainability.
Leverage existing component styles - Before writing new styles, check if similar functionality already exists in existing components. Reuse or extend existing styles rather than duplicating them.
Consider all affected elements systematically - When implementing CSS changes (especially broad ones like accessibility features), identify and address all related UI elements that should be modified consistently.
Example of good organization:
// motion-control.scss - dedicated file for motion-related overrides
@media (prefers-reduced-motion: reduce) {
.fa-spin,
.icon.spin,
.status-icon--spin,
.argo-button,
.application-resource-tree__node-animation,
[class*="sliding-panel"],
.argo-dropdown__content,
.tippy-popper *,
.Toastify--animate {
animation: none !important;
transition: none !important;
}
}
This approach keeps styles organized, reduces code duplication, and makes the codebase more maintainable.
Select the right synchronization mechanism based on your specific use case to avoid performance regressions and ensure thread safety. Follow these guidelines:
Context Management: Always call defer cancel() immediately after creating a cancellable context to ensure proper cleanup:
ctx, cancel := context.WithCancel(c.Context())
defer cancel()
Mutex Selection: Prefer regular sync.Mutex over sync.RWMutex unless you have a read-heavy workload. RWMutex can be less efficient and cause performance regressions in typical scenarios.
Atomic Operations: Use modern atomic types like atomic.Int64 instead of manual atomic operations on primitive types for better safety and readability.
Test Synchronization: Use channels or other explicit synchronization mechanisms in tests to avoid flakiness, rather than relying on timing:
done := make(chan struct{})
go func() {
// test logic
close(done)
}()
<-done // wait for completion
Race Condition Prevention: In timeout scenarios with context cancellation, ensure proper ordering of operations and consider using locks when multiple goroutines access shared state concurrently.
Security configurations should use explicit controls and secure defaults rather than implicit permissions or hardcoded credentials. This principle helps prevent accidental privilege escalation and credential exposure.
Key practices:
Example of explicit RBAC configuration:
# Explicit logs permission instead of implicit access via applications.get
p, dev-team, logs, get, my-app/*, allow
p, dev-team, applications, get, my-app/*, allow
# Explicit override permission with secure default (disabled)
application.sync.externalRevisionConsideredOverride: 'true'
p, dev-team, applications, override, my-app/*, allow
Example of avoiding hardcoded secrets:
# Instead of hardcoding in manifests:
# oidc.clientSecret: "hardcoded-secret-value"
# Use kubectl or external secret management:
kubectl create secret generic argocd-secret \
--from-literal=oidc.clientSecret="your-secret-value"
This approach reduces security risks by making security boundaries explicit and preventing accidental exposure of sensitive information.
Always validate and sanitize user-provided inputs to prevent injection attacks, particularly path traversal vulnerabilities. User inputs should be treated as untrusted and validated against expected patterns before use in file operations, URL construction, or system commands.
Key areas requiring validation:
../../../etc/passwdsecurejoin) when dealing with user-provided paths that could contain symlinks or relative path componentsExample of vulnerable code:
// Vulnerable - no validation
args = append(args, fmt.Sprintf("%s/%s-%s.tgz", repo, chartName, version))
// Better - with validation
if !isValidRepoName(repo) || !isValidChartName(chartName) || !isValidVersion(version) {
return "", fmt.Errorf("invalid input parameters")
}
args = append(args, fmt.Sprintf("%s/%s-%s.tgz", repo, chartName, version))
Implement input validation early in the request processing pipeline and use allowlists rather than denylists when possible. Consider using established validation libraries rather than implementing custom validation logic.
Always use structured logging methods like klog.InfoS and klog.ErrorS instead of unstructured methods like klog.Infoln, klog.Infof, or klog.Errorln. Structured logging improves log parsing, analysis, and monitoring capabilities. Additionally, ensure log messages are clear, descriptive, and provide complete context about actions taken and values being set.
Key requirements:
Example transformation:
// Bad - unstructured logging
klog.Infoln("pod=", pod.Name, "scoreMap=", gs.ScoreMap[pod.Name])
klog.ErrorS(nil, "loadPlugin", "pathxxx", pluginPath) // wrong level for non-error
// Good - structured logging
klog.InfoS("Scoring node for pod", "pod", pod.Name, "score", gs.ScoreMap[pod.Name])
klog.InfoS("Loading plugin", "path", pluginPath)
// Bad - unclear message
klog.V(3).Infof("node %s is not ready,need continue", n.Name)
// Good - clear, descriptive message
klog.V(3).InfoS("Node is not ready/unschedulable, skipping node", "node", n.Name)
When modifying constraints in database migrations, follow a safe sequence of operations to prevent integrity errors:
def upgrade():
dialect_name = op.get_bind().dialect.name
# Add default values to referenced table
if dialect_name == "postgresql":
op.execute("INSERT INTO dag_bundle (name) VALUES ('dags-folder') ON CONFLICT (name) DO NOTHING;")
elif dialect_name == "mysql":
op.execute("INSERT IGNORE INTO dag_bundle (name) VALUES ('dags-folder');")
elif dialect_name == "sqlite":
op.execute("INSERT OR IGNORE INTO dag_bundle (name) VALUES ('dags-folder');")
# Modify constraints safely
with op.batch_alter_table("dag", schema=None) as batch_op:
# 1. Populate data first
conn = op.get_bind()
conn.execute(text("UPDATE dag SET bundle_name = 'dags-folder' WHERE bundle_name IS NULL"))
# 2. Drop FK constraint before changing nullability
batch_op.drop_constraint("dag_bundle_name_fkey", type_="foreignkey")
# 3. Alter column nullability
batch_op.alter_column("bundle_name", nullable=False, existing_type=sa.String(length=250))
# 4. Recreate foreign key after both sides are compatible
with op.batch_alter_table("dag", schema=None) as batch_op:
batch_op.create_foreign_key(
"dag_bundle_name_fkey", "dag_bundle", ["bundle_name"], ["name"]
)
For downgrades, restore schema constraints but avoid destructive data operations when possible.
Adhere to Go naming conventions and maintain consistency with established codebase terminology. This includes proper capitalization of acronyms (e.g., “OCI” not “Oci”), avoiding underscores in identifiers, following standard library naming patterns, and using consistent terminology throughout the codebase.
Key guidelines:
GetOCICreds() not GetOciCreds()unmarshalYamlFile not unMarshalYamlFile (consistent with json.Unmarshal)APIVersion over groupVersion when it aligns with backend terminology_seconds suffixExample:
// Good - follows Go conventions
func (repo *Repository) GetOCICreds() oci.Creds {
// implementation
}
// Bad - incorrect acronym capitalization
func (repo *Repository) GetOciCreds() oci.Creds {
// implementation
}
Consistent naming reduces cognitive load, improves code readability, and aligns with community expectations and tooling requirements.
When CI/CD pipelines make automated commits (such as image bumps or manifest updates), use standardized git commit trailers to maintain traceability back to the original code changes. This enables developers and tools to trace deployment artifacts to their source commits.
Use consistent trailer naming conventions in your CI scripts. Include essential metadata like author, SHA, message, repository URL, and date in ISO 8601 format:
git commit -m "Bump image to v1.2.3" \
--trailer "Argocd-related-commit-author: Author Name <author-email>" \
--trailer "Argocd-related-commit-sha: <code-commit-sha>" \
--trailer "Argocd-related-commit-message: Commit message of the code commit" \
--trailer "Argocd-related-commit-repourl: https://git.example.com/owner/repo" \
--trailer "Argocd-related-commit-date: 2025-06-09T13:50:18-04:00"
This approach is particularly important for deployment pipelines where automated tooling pushes changes to DRY manifests after code changes, ensuring full audit trails and enabling effective debugging of deployment issues.
Always use dedicated configuration files for specific configuration needs rather than modifying core build or system files. This practice improves maintainability, traceability, and scope control.
When working with configuration:
buf.yaml for build ignores) instead of modifying core build files like MakefilesExample:
# Don't do this:
# In Makefile
ci-build-misc: \
clean-tools \
proto \
go-generate \
# buf-breaking was removed here
# Instead do this:
# 1. Keep the Makefile entry
ci-build-misc: \
clean-tools \
proto \
go-generate \
buf-breaking \ # Don't remove this, add ignores to proto/internal/buf.yaml instead
# 2. Add specific ignores in the dedicated configuration file (buf.yaml)
# 3. For tool dependencies, use specific versions:
WORKFLOWCHECK := $(LOCALBIN)/workflowcheck
$(WORKFLOWCHECK): | $(LOCALBIN)
$(call go-install-tool,$(WORKFLOWCHECK),go.temporal.io/sdk/contrib/tools/workflowcheck,v0.3.0)
This approach keeps configuration changes properly scoped, makes them easier to track, and prevents unintended consequences from global modifications or unexpected version changes.
Documentation tooling and configuration files should leverage shared/centralized resources rather than maintaining duplicate copies across repositories. This reduces maintenance burden, ensures consistency, and simplifies updates.
Examples:
# Instead of separate requirements.txt files:
# sphinx>=5.0
# sphinx-autoapi>=1.8
# sphinx-airflow-theme>=0.2.2
# Use pyproject.toml with direct references when needed:
[project.optional-dependencies]
docs = [
"sphinx>=5.0",
"sphinx-autoapi>=1.8",
"sphinx_airflow_theme @ https://github.com/apache/airflow-site/releases/download/0.2.3/sphinx_airflow_theme-0.2.3-py3-none-any.whl",
]
# Use shared mechanism instead of duplicating large wordlists
# uv run --group docs build-docs
# (will automatically use common spelling exclusion file from `airflow/docs`)
This approach ensures documentation tools stay consistent across projects and simplifies maintenance when standards change.
Ensure that configuration user interfaces accurately reflect the underlying configuration behavior and data structures. UI controls should provide clear, transparent mapping to the actual configuration values, including precedence rules and field relationships.
When building configuration forms, avoid situations where:
For example, if your backend has precedence rules like “ValuesObject takes precedence over Values”, the UI should either:
// Bad: UI checkbox maps to multiple backend states
const isEnabled = automated.enabled !== false; // Could be undefined, true, or false
// Better: Explicit handling of configuration states
const getAutoSyncState = (automated) => {
if (automated.enabled === false) return 'disabled';
if (automated.enabled === true) return 'enabled';
return 'default-enabled'; // When enabled field is undefined
};
This prevents user confusion and ensures that configuration changes through the UI produce predictable results in the underlying system.
Define and use domain-specific error types within bounded contexts rather than relying on generic service errors. Each component/package should define its own error types that reflect its domain concepts, avoiding dependencies on external error frameworks.
Key points:
Example:
// Bad - using service errors in application code
func (n *Node) validateCollection(field string) error {
if field.val.Kind() != reflect.Map {
return serviceerror.NewInternal("invalid collection type")
}
return nil
}
// Good - using domain-specific errors
type ValidationError struct {
Field string
Reason string
}
func (e *ValidationError) Error() string {
return fmt.Sprintf("validation failed for %s: %s", e.Field, e.Reason)
}
func (n *Node) validateCollection(field string) error {
if field.val.Kind() != reflect.Map {
return &ValidationError{
Field: field,
Reason: "must be a map type",
}
}
return nil
}
// Convert domain errors to service errors at system boundaries
func (s *Service) handleRequest() error {
err := domain.Operation()
if err != nil {
// Convert domain errors to appropriate service errors
var valErr *ValidationError
if errors.As(err, &valErr) {
return serviceerror.NewInvalidArgument(valErr.Error())
}
return serviceerror.NewInternal("unexpected error")
}
return nil
}
Add nil and empty checks at the beginning of functions to prevent downstream issues and avoid unnecessary processing. This pattern improves code safety and performance by failing fast when inputs are invalid or empty.
Key practices:
Example:
func buildInitialMetadata(metadata []*meshconfig.MeshConfig_ExtensionProvider_HttpHeader) []*core.HeaderValue {
if metadata == nil {
return nil
}
target := make([]*core.HeaderValue, 0, len(metadata))
// ... rest of processing
}
// Check collection length before operations
if len(ipRange) > 0 {
chains = append(chains, directChain)
// ... process ipRange
}
// Short-circuit nil checks to clean up nested logic
if wh.nodes == nil {
// handle nil case early
return
}
This approach prevents null pointer exceptions, reduces cognitive complexity, and makes code more maintainable by handling edge cases upfront rather than deep within the logic.
Use the most specific and descriptive assertion methods available in your test framework to improve test readability and failure diagnostics. Prefer methods that clearly state intent and produce helpful error messages.
Examples:
require.Empty(t, collection) instead of require.Len(t, collection, 0)require.NotZero(t, count) instead of require.True(t, count > 0)assert.EqualExportedValues(t, expected, actual) instead of proto.Equal() to get helpful diffs on failurefunc TestCollectionAttributes(t *testing.T) {
// Define test cases for both string and int keys
testCases := []struct{
name string
keyType string
expected interface{}
}{
{"StringKey", "string", expectedStringResult},
{"IntKey", "int", expectedIntResult},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Test logic with tc.keyType and tc.expected
})
}
}
Also use descriptive test names that follow a pattern like TestComponent_Behavior_Condition (e.g., TestLogger_FilesArentRotated_WhenDisabled) to clearly communicate what’s being tested.
These practices make tests more maintainable, easier to debug when they fail, and the intent clearer to other developers.
Carefully manage metric label cardinality to prevent excessive memory usage and maintain system performance. Follow these guidelines:
Example:
// DON'T: High cardinality labels on latency metrics
metrics.OperationLatency.With(metricsHandler).
WithTags(metrics.NamespaceTag(namespace)).Record(elapsed)
// DO: Separate counter for namespace-level tracking
metricsHandler = metricsHandler.WithTags(metrics.TargetClusterTag(clusterName))
if len(namespaceName) != 0 {
// Add namespace tag only to counter metrics
metricsHandler = metricsHandler.WithTags(metrics.NamespaceTag(namespaceName))
}
metrics.OperationCount.With(metricsHandler).Record(1)
Use dynamic configuration selectively and effectively by following these principles:
// Use dynamic config for runtime-configurable values SlowRequestThreshold: dc.GetDurationProperty( dynamicconfig.SlowRequestLogThreshold, 5 * time.Second, )
2. Document config parameters with clear impact descriptions:
```go
// Bad: Unclear impact
MaxRetryAttempts = NewGlobalIntSetting(
"workflow.maxRetryAttempts",
5,
"Maximum retry attempts allowed"
)
// Good: Clear threshold impact
MaxRetryAttempts = NewGlobalIntSetting(
"workflow.maxRetryAttempts",
5,
"Maximum retry attempts allowed. When exceeded, workflow fails permanently"
)
// Good: Injecting specific config function func NewHandler(thresholdFn dynamicconfig.DurationPropertyFn) { threshold := thresholdFn() }
This approach improves testability, maintainability, and makes configuration dependencies explicit while ensuring configs are used only where runtime modification is necessary.
---
## Document technical details clearly
<!-- source: volcano-sh/volcano | topic: Documentation | language: Markdown | updated: 2025-05-27 -->
Ensure all technical documentation provides clear explanations and follows proper formatting conventions. Technical parameters, configuration options, and complex concepts should include explanatory comments or descriptions to make them accessible to users and maintainers.
Key practices:
- Add explanatory comments for configuration parameters and technical arguments
- Follow established formatting conventions (e.g., proper author name formatting in citations)
- Keep documentation focused on its intended scope (design docs should focus on concepts, not implementation details)
- Provide context and meaning for technical settings rather than just listing them
Example from configuration documentation:
```yaml
- name: deviceshare
arguments:
deviceshare.VGPUEnable: true # Enable virtual GPU support for workloads
deviceshare.KnowGeometriesCMName: volcano-vgpu-device-config # ConfigMap containing GPU geometry definitions
This approach ensures documentation serves its primary purpose: helping users understand and correctly use the system.
When writing documentation that involves handling sensitive data or security-related operations, always include explicit warnings about security implications and provide references to security best practices. Users may not be aware of default security behaviors that could expose sensitive information.
For example, when documenting the creation of Kubernetes Secrets, include a warning that Secrets are unencrypted by default:
kubectl create secret generic ufm-credentials \
--from-literal=username='your-ufm-username' \
--from-literal=password='your-ufm-password' \
-n volcano-system
Warning: Secrets are still unencrypted by default. If users need to encrypt Secrets, please refer to: https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/
This practice helps users make informed security decisions and prevents accidental exposure of sensitive data due to lack of awareness about default security configurations.
Implement secure configuration practices to protect sensitive data and prevent security vulnerabilities. This includes three key areas:
Credential Protection: Never store or transmit passwords and sensitive credentials in plain text. Use encryption tools, secure credential management systems, or environment variables with proper access controls.
Authentication Handling: When multiple authentication methods are supported, implement clear precedence rules to prevent conflicts and failures. For example, if both basic auth and token auth are configured, prioritize one method and clear the other to avoid authentication errors.
Secure Defaults: Disable debug endpoints, profiling tools, and other potentially sensitive features by default. Require explicit opt-in through configuration flags to enable these features.
Example of secure authentication handling:
tConf := &transport.Config{
Username: p.conf["username"],
Password: p.conf["password"],
BearerToken: p.conf["bearertoken"],
}
// If basic auth is set, token will be cleared to avoid conflicts
if tConf.Username != "" && tConf.Password != "" {
tConf.BearerToken = ""
tConf.BearerTokenFile = ""
}
Example of secure defaults:
fs.BoolVar(&s.EnablePprof, "enable-pprof", false, "Enable the pprof endpoint; it is false by default")
These practices help prevent security audit issues, reduce attack surface, and ensure that sensitive functionality requires deliberate activation rather than accidental exposure.
When propagating errors up the call stack, always wrap them with contextual information about the failing operation using fmt.Errorf with the %w verb. This practice significantly improves debugging by providing a clear trail of what operations were being performed when the error occurred.
The pattern should be: return fmt.Errorf("description of what failed: %w", err)
Example from the codebase:
// Instead of:
if err != nil {
return err
}
// Do this:
if err != nil {
return fmt.Errorf("failed to initialize oci client: %w", err)
}
// Or for more specific context:
objsMap, err := ctrl.getPermittedAppLiveObjects(destCluster, app, proj, projectClusters)
if err != nil {
return fmt.Errorf("error getting permitted app live objects: %w", err)
}
This approach maintains the original error chain (allowing errors.Is and errors.As to work correctly) while adding valuable context about the operation that failed. It transforms generic errors into actionable debugging information, making it easier to identify the root cause and the sequence of operations that led to the failure.
Carefully review all security permissions and privileges to ensure they follow the principle of least privilege. Avoid coupling unrelated permissions together and verify that elevated privileges are truly necessary.
When defining RBAC roles, separate concerns appropriately rather than bundling unrelated permissions. For example, avoid coupling PriorityClass permissions with job management permissions when they serve different purposes and can be granted through separate mechanisms.
When granting elevated security capabilities like DAC_OVERRIDE, SETUID, SETGID, or SETFCAP, explicitly verify and document why these privileges are required. Consider the security implications and ensure proper justification.
Example of proper separation:
# Good: Focused role for vcjob management only
rules:
- apiGroups: ["batch.volcano.sh"]
resources: ["jobs"]
verbs: ["create", "get", "list", "update", "delete"]
# PriorityClass permissions handled separately through other roles
Example of privilege verification:
# Document and verify elevated capabilities
securityContext:
capabilities:
add: ["DAC_OVERRIDE", "SETUID", "SETGID", "SETFCAP"]
# Verified: These capabilities are required for [specific functionality]
When implementing security features that involve access control, authentication, or authorization, carefully validate that the changes don’t inadvertently grant broader access than intended. This is particularly critical when dealing with secret access, trust domains, or credential handling.
Key areas to scrutinize:
Example from the codebase:
// PROBLEMATIC: Allows proxy to access secrets for any WasmPlugin they request
wasmPlugins := push.WasmPluginsByName(proxy, core.ParseExtensionName(resourceNames))
// BETTER: Only allow access to WasmPlugins that actually apply to the proxy
wasmPlugins := push.WasmPlugins(proxy)
Always ask: “Could this change allow a client to access resources they shouldn’t have access to?” If the answer is unclear, implement additional validation or use more restrictive approaches like RBAC-style permission checks.
When implementing network topology discovery features, use discovery methods that are appropriate for the specific network protocol being used. Different network protocols have different architectures and require different approaches for topology discovery.
For centralized management protocols like UFM (Unified Fabric Manager), use endpoint-based configuration to connect to the central management interface. For distributed protocols like RoCE (RDMA over Converged Ethernet), use distributed discovery methods such as LLDP (Link Layer Discovery Protocol) to collect neighbor information from each node and switch, then reconstruct the full topology.
For InfiniBand networks, leverage tools like ibnetdiscover for topology discovery. For RoCE environments, consider using LLDP-based tools like lldpd or similar open-source alternatives on the node side.
Example configuration showing protocol-appropriate approaches:
discoveryConfig:
- source: ufm # Centralized management
enabled: true
config:
endpoint: https://ufm-server:8080
username: admin
password: password
- source: roce # Distributed discovery
enabled: true
config:
# Uses LLDP-based discovery, may require daemonset deployment
discoveryMethod: lldp
Be aware that switch support and access methods can vary significantly between vendors, which may require vendor-specific contributions or adaptations for full compatibility.
Choose names that accurately describe their purpose and avoid unnecessary verbosity. Field names should semantically match what they contain or validate, and component names should be concise without redundant prefixes that don’t add clarity.
For example, when a field contains a validation pattern, use “format” instead of “type”:
-- Instead of:
["type"] = "^[0-9]*$",
-- Use:
["format"] = "^[0-9]*$",
Similarly, avoid redundant prefixes in component names:
# Instead of:
workload='argocd-repo-server'
# Use:
workload='repo-server'
This makes code more intuitive to read and components easier to find and reference.
Ensure that user actions and validations provide clear feedback rather than failing silently. When operations cannot be performed, disable UI elements or provide explicit error messages to inform users of the system state.
For UI interactions, disable buttons or controls when the underlying operation cannot be executed:
{
iconClassName: classNames('fa fa-redo', {'status-icon--spin': !!refreshing}),
title: <ActionMenuItem actionLabel='Invalidate Cache' />,
disabled: !!refreshing, // Prevent silent ignoring of clicks
action: () => {
if (!refreshing) {
// perform action
}
}
}
For validation scenarios, implement comprehensive validation that returns specific error messages for each field:
validate: vals => {
return (action.params || []).reduce((acc, param) => {
acc[param.name] = vals[param.name] && vals[param.name].match(param.type) ?
undefined : `required format: ${param.type}`;
return acc;
}, {});
}
This approach prevents user confusion and ensures that all failure scenarios are communicated clearly rather than being silently ignored.
Implement proper concurrency control mechanisms to prevent resource exhaustion and ensure thread-safe operations. Use sync.Once for thread-safe initialization to avoid memory leaks, and employ goroutine pools or semaphores to limit concurrent operations instead of spawning unlimited goroutines.
For initialization that should happen only once:
var volumeBindingPlugin *vbcap.VolumeBinding
var once sync.Once
once.Do(func() {
plugin, err := vbcap.New(context.TODO(), vbArgs.VolumeBindingArgs, handle, features)
if err == nil {
volumeBindingPlugin = plugin.(*vbcap.VolumeBinding)
}
})
For controlled concurrent processing:
// Use goroutine pool instead of unlimited goroutines
semaphore := make(chan struct{}, 16)
var wg sync.WaitGroup
for _, taskInfo := range job.TaskStatusIndex[status] {
wg.Add(1)
semaphore <- struct{}{} // Acquire semaphore
go func(task *TaskInfo) {
defer func() {
<-semaphore // Release semaphore
wg.Done()
}()
// Process task
}(taskInfo)
}
wg.Wait()
This approach prevents memory leaks from repeated initialization, avoids overwhelming the system with too many concurrent operations, and ensures predictable resource usage patterns.
When defining API types, enums, or interfaces, ensure they represent accurate conceptual distinctions within the domain model rather than implementation details or delivery mechanisms. Each type should correspond to a meaningful category that users and developers can clearly understand and differentiate.
Before adding new values to type unions or enums, verify that the addition represents a true conceptual category. For example, avoid conflating delivery mechanisms (like OCI repositories) with content types (like Helm charts or Kustomize configurations).
Example of problematic type definition:
// Problematic - mixes content types with delivery mechanisms
export type AppSourceType = 'Helm' | 'Kustomize' | 'Directory' | 'Plugin' | 'OCI';
Example of proper type definition:
// Better - focuses on actual source content types
export type AppSourceType = 'Helm' | 'Kustomize' | 'Directory' | 'Plugin';
When designing interfaces, include only fields that serve the specific purpose while maintaining conceptual clarity. This approach prevents API confusion and ensures that type definitions remain meaningful and maintainable over time.
Ensure configurations are contextually appropriate and properly validated for their intended use case. This includes verifying that configuration options are suitable for the target resource type, handling configuration state transitions correctly, and choosing appropriate dependencies.
Key considerations:
Example of proper configuration validation:
function toggleAutoSync(obj)
if obj.spec.syncPolicy and obj.spec.syncPolicy.automated then
-- Check current state before transition
if obj.spec.syncPolicy.automated.enabled then
obj.spec.syncPolicy.automated.enabled = false
else
obj.spec.syncPolicy.automated = nil
end
end
end
This practice prevents configuration mismatches, reduces runtime errors, and ensures that settings are applied appropriately for their intended context.
When working with Temporal workflows, maintaining deterministic behavior is critical for reliable replay and execution across distributed systems.
Key practices:
// Instead of this:
now := env.Now()
nowpb := timestamppb.New(now)
// Prefer this when possible:
now := request.GetTriggerTime().AsTime()
nowpb := timestamppb.New(now)
// Instead of directly comparing times:
if t.Now().After(deadline) { ... }
// Use utilities that handle skew:
if queues.IsTimeExpired(deadline, t.Now()) { ... }
Be careful with non-deterministic functions: Common libraries may contain non-deterministic behavior. Verify that operations like proto.Equal are safe for workflow code or create deterministic alternatives.
Maintain time consistency: When passing time values between components, ensure they derive from the same source to avoid inconsistency in execution paths.
These practices ensure workflows execute consistently during replay, across different clusters in multi-region setups, and when recovering from failures.
Always validate environment variables and provide appropriate fallbacks when using them for configuration. Environment variables should be checked for existence and non-empty values before use, with clear fallback behavior defined.
When reading environment variables, use proper validation patterns:
// Good: Check for existence and non-empty value
if cacheHome := os.Getenv("XDG_CACHE_HOME"); cacheHome != "" {
base = cacheHome
} else {
// Provide clear fallback
base = filepath.Join(os.Getenv("HOME"), ".cache")
}
// Good: Use flag defaults from environment variables
func addProjectFlags(cmd *cobra.Command) {
cmd.Flags().StringVar(&progressMode, "progress",
getEnvOrDefault("COMPOSE_PROGRESS", "auto"),
"Set type of progress output")
}
For experimental features, use descriptive environment variable names with clear prefixes:
if _, ok := os.LookupEnv("COMPOSE_EXPERIMENTAL_INCLUDE_REMOTE"); ok {
// Enable experimental feature
}
Establish clear precedence rules for configuration sources (command line flags > environment variables > config files > defaults) and document the expected behavior when multiple sources are present. Always handle the case where environment variables might be set but empty, as this often indicates intentional override behavior.
API docs and interface examples must be unambiguous and cover defaults.
Apply this standard when writing or updating API documentation:
? to start the query and & for every additional parameter. Avoid ambiguous/incorrect examples like multiple ? in the same query string.Example (query param clarity):
# Correct: one '?' then '&' for subsequent params
result_backend = 'gs://mybucket/some-prefix?gcs_project=myproject&firestore_project=myproject2&ttl=600'
# Wrong/ambiguous: multiple '?'
# result_backend = 'gs://...?...?firestore_project=...'
Example (default + collision behavior):
sig = add.si(2, 2)
g = group(sig, add.si(3, 3))
# Show default stamping behavior (no custom stamp)
# g.stamp(...)
# Show repeated-key behavior explicitly
g.stamp(stamp='your_custom_stamp')
meta = sig.freeze()._get_task_meta()
This reduces user confusion and support load by ensuring examples match actual API semantics and defaults.
When interacting with APIs, minimize network requests and improve code maintainability by:
Example:
# Create a reusable function for API calls
call() {
curl -fL -X POST -H "Accept: application/vnd.github.v3+json" -H "Authorization: token $PAT" "$@"
}
# Use the function and make a single API call to get multiple pieces of data
result=$(call "https://api.github.com/repos/$PARENT_REPO/actions/runs/$run_id")
status=$(echo "$result" | jq -r .status)
conclusion=$(echo "$result" | jq -r .conclusion)
Design APIs to support multiple parameters from the beginning rather than limiting them to single parameter inputs. This prevents future refactoring needs and provides better extensibility for complex operations.
When designing resource action APIs or similar interfaces, structure them to accept parameter collections rather than single values. Instead of hardcoding single parameter handling:
// Avoid: Limited to single parameter
const resourceActionParameters = action.hasParameters ?
[{name: action.name, value: vals.inputParameter}] : [];
Design for multiple parameters using parameter mapping:
// Preferred: Support multiple parameters
const resourceActionParameters = action.params
? action.params.map(param => ({
name: param.name,
value: vals[param.name] || param.default,
type: param.type,
default: param.default
}))
: [];
This approach allows APIs to evolve naturally as requirements grow, supports complex operations that need multiple inputs, and provides a consistent interface pattern across different API endpoints. Consider the parameter structure in your API specification from the start, even if initially only one parameter is needed.
When implementing functionality that involves multiple possible algorithmic approaches, explicitly analyze the complexity tradeoffs between alternatives before making a decision. Consider factors such as:
Document your reasoning to make the decision process clear to reviewers and future maintainers.
Example:
// Approach chosen: Store component status directly in the task
// Considered alternatives:
// 1. Maintain timestamp list in mutable state
// Pros: No need for versioned_transition_offset field
// Cons: More complex updates when nodes/tasks are removed
// Requires checking entire tree or maintaining reference counts
// Additional write operations at task processing time
// 2. Current approach with direct status field
// Pros: Simpler update logic
// versioned_transition_offset provides additional benefits
// for referencing specific component tasks
// Cons: Requires carrying over status during replication
Use the appropriate Docker network API method based on the specific behavior you need. NetworkList and NetworkInspect have different matching behaviors that can lead to unexpected results if used incorrectly.
Key distinctions:
NetworkList with name filter performs reliable exact matching when combined with post-filteringNetworkInspect performs prefix matching on both name and ID, which can cause ambiguous matchesNetworkDisconnect over container removal to preserve associated resources like anonymous volumesExample of proper network resolution:
// Use NetworkList for exact name matching
networks, err := s.apiClient().NetworkList(ctx, moby.NetworkListOptions{
Filters: filters.NewArgs(filters.Arg("name", networkName)),
})
if err != nil {
return err
}
// Filter for exact match since NetworkList can return partial matches
networks = utils.Filter(networks, func(net moby.NetworkResource) bool {
return net.Name == networkName
})
// Check for proper labels to ensure it's the expected network
for _, net := range networks {
if net.Labels[api.ProjectLabel] == expectedProjectLabel &&
net.Labels[api.NetworkLabel] == expectedNetworkLabel {
return nil
}
}
This approach prevents issues like matching a network named db to a network with ID db9086999caf and ensures you’re working with the intended network resource.
When designing Protocol Buffer APIs, use oneof fields to create extensible message structures that maintain compatibility while providing type safety. This pattern allows APIs to evolve over time by adding new message variants without breaking existing clients.
Instead of creating separate message types that require type checking during deserialization:
// Avoid this pattern
message ChasmTransferTaskInfo {
// fields...
}
// And then manually figuring out which message to deserialize into
Use the oneof pattern to create a clear type hierarchy within a single message:
message OutboundTaskInfo {
// Common fields...
string destination = 7;
oneof task_details {
// Task-specific details
StateMachineTaskInfo state_machine_info = 8;
ChasmTaskInfo chasm_info = 9;
// Future task types can be added here
}
}
This approach offers several benefits:
Remember that while this is binary compatible at the protocol level, generated code in some languages (like Go) might have API-breaking changes, so coordinate with client teams when making these changes.
Ensure API validation rules and type definitions remain consistent across all validation layers and components. When implementing validation logic, verify that kubebuilder annotations, manual validation functions, and API type definitions all enforce the same constraints and use compatible data types.
For example, if kubebuilder validation allows both HyperNode and Node member types, manual validation should also distinguish between these types consistently:
// API definition should use appropriate types
type MemberType string
// Manual validation should match kubebuilder rules
func validateHyperNodeMemberSelector(selector hypernodev1alpha1.MemberSelector, fldPath *field.Path, memberType hypernodev1alpha1.MemberType) field.ErrorList {
if memberType != hypernodev1alpha1.MemberTypeHyperNode && memberType != hypernodev1alpha1.MemberTypeNode {
err := field.Invalid(fldPath, memberType, "hyperNode member type must be one of HyperNode or Node")
errs = append(errs, err)
return errs
}
// Validation logic should distinguish between member types consistently
if memberType == hypernodev1alpha1.MemberTypeHyperNode {
// HyperNode-specific validation that matches kubebuilder rules
}
}
Additionally, choose appropriate data types in API definitions. Use strongly-typed fields (like int for numeric values) rather than strings when the semantic meaning is numeric, unless there’s a specific need for named values that require string-to-numeric mapping.
Establish clear guidelines for feature flag creation, default values, and removal strategy based on risk assessment and user impact.
When to use feature flags:
Default value strategy:
Lifecycle management:
Example pattern:
// Good: Clear intent and lifecycle plan
NativeMetadataExchange = env.Register("NATIVE_METADATA_EXCHANGE", true,
"Enable native metadata exchange filter. Default true for escape hatch during rollout.").Get()
// Avoid: Unclear necessity
SomeInternalRefactor = env.Register("ENABLE_INTERNAL_REFACTOR", false,
"Enable internal refactoring that users don't interact with").Get()
This approach ensures feature flags serve their intended purpose without becoming permanent configuration bloat.
When documenting or describing security-related changes, especially bug fixes and behavioral modifications, ensure clear and explicit communication of security implications. Use proper security terminology and clearly describe both the problematic behavior and the fix.
Key requirements:
Example of unclear security communication:
releaseNotes:
- |
**Fixed** For a WasmPlugin of type FAIL_CLOSE, if the wasm image fetch fails, a DENY-ALL RBAC filter will be used.
Example of clear security communication:
releaseNotes:
- |
**Fixed** an issue where if a wasm image fetch fails, an allow all RBAC filter is used. Now if `failStrategy` is set to `FAIL_CLOSE`, a DENY-ALL RBAC filter will be used.
This practice helps developers understand security implications, reduces confusion about security behaviors, and ensures that security fixes are properly communicated to users who need to understand the impact on their systems.
When introducing, modifying, or removing observability features (tracing, metrics, monitoring configurations), always include clear documentation explaining the rationale, use cases, and migration guidance. Users need to understand why they would want to use a feature and how it solves their specific problems.
For new features, explain the problem being solved and provide usage examples. For configuration changes, document the migration path and any behavioral differences. For removals, clearly state alternatives and upgrade steps.
Example from release notes:
releaseNotes:
- |
**Added** environment variable `PILOT_SPAWN_UPSTREAM_SPAN_FOR_GATEWAY`,
which separates tracing span for both server and client gateways.
This currently defaults to false but will become default in the future.
Use this when you need separate visibility into gateway request
processing stages for better debugging and performance analysis.
This approach helps teams make informed decisions about their observability stack and reduces confusion during upgrades or configuration changes.
When configuring dependencies in Dockerfiles and other configuration files, use the latest patch versions while keeping major and minor versions fixed. Patch versions typically contain important security fixes and bug corrections without introducing breaking changes. For example, prefer golang:1.23.7 over golang:1.23.0, as “small versions will fix some errors.” This approach balances stability with security by avoiding breaking changes from major/minor updates while ensuring you receive critical fixes. Apply this consistently across all configuration files in your project.
Example:
# Good - uses latest patch version
FROM golang:1.23.7 AS builder
# Avoid - missing important patch fixes
FROM golang:1.23.0 AS builder
When delegating work to concurrent/asynchronous execution (tasks, workers, background processes), ensure: 1) State ordering: the async code cannot observe missing/partial state. For DB-backed workflows, enqueue/start tasks only after the transaction commits. 2) Lifecycle safety: concurrent worker processes must be cleaned up when the parent/master process exits so you don’t leave orphaned workers running.
Example (transaction commit before enqueue):
from django.db import transaction
def create_article_and_process(request):
article = Article(...) # fill fields
with transaction.atomic():
article.save()
def enqueue_task():
process_article.delay(article.pk)
transaction.on_commit(enqueue_task)
return response_ok()
Checklist:
transaction.on_commit) or equivalent “after commit” mechanism.Always validate and sanitize external URLs before opening them or using them in navigation operations to prevent URL injection attacks. Implement domain allowlists to restrict which external domains are permitted, and avoid directly passing user-controlled or external URL data to functions like window.open() without proper validation.
Example of vulnerable code:
// Dangerous - direct use of external URL
onClick={e => {
e.stopPropagation();
window.open(m.imageUrl); // Potential injection point
}}
Instead, implement URL validation:
const isAllowedDomain = (url: string) => {
const allowedDomains = ['trusted-domain.com', 'another-trusted.com'];
try {
const urlObj = new URL(url);
return allowedDomains.includes(urlObj.hostname);
} catch {
return false;
}
};
onClick={e => {
e.stopPropagation();
if (isAllowedDomain(m.imageUrl)) {
window.open(m.imageUrl);
}
}}
Consider removing external URL functionality entirely if proper validation cannot be implemented immediately, and add it back as a follow-up enhancement with appropriate security measures.
Combine multiple nested if statements using logical operators (and/or) to improve code readability and reduce unnecessary indentation. Remove redundant nil checks when they are unnecessary, especially when subsequent checks would naturally handle the nil case.
Instead of deeply nested conditions:
if obj.status ~= nil then
if obj.status.conditions ~= nil then
for i, condition in ipairs(obj.status.conditions) do
if condition.type ~= nil then
if condition.type == "Ready" then
if condition.status ~= nil and condition.reason ~= nil then
-- logic here
end
end
end
end
end
end
Consolidate into cleaner, more readable code:
if obj.status and obj.status.conditions then
for i, condition in ipairs(obj.status.conditions) do
if condition.type == "Ready" and condition.status == "True" and condition.reason == "SuccessfulCreateOrUpdate" then
-- logic here
end
end
end
This approach reduces cognitive load, eliminates unnecessary nesting levels, and makes the code’s intent clearer by focusing on the actual conditions that matter rather than defensive nil checking.
Do not install privilege escalation tools like sudo, su, or doas in containers unless they are explicitly required for the application’s functionality. Most containers run as root by default, making sudo redundant and potentially creating security vulnerabilities by expanding the attack surface.
Before adding privilege escalation tools, consider:
Example of what to avoid:
# Unnecessary - container already runs as root
RUN apt-get update && \
apt-get install -y sudo
Example of better approach:
# Perform operations directly as root during build
RUN apt-get update && \
apt-get install -y required-package
This practice reduces the container’s attack surface and follows the principle of least privilege by not providing unnecessary tools that could be exploited by attackers.
Tests should be deterministic and explicit in their assertions to ensure reliability and maintainability. Follow these guidelines:
Example - Instead of:
assert.Assert(t, strings.Contains(output, "Skipped"))
w.Write([]byte("hello"))
Better approach:
// Use explicit assertions
assert.DeepEqual(t, lines, []string{"hello", "world!"})
// Use testing utilities
dirName := t.TempDir()
// Use controlled test data
internal.Version = "v9.9.9-test"
// Test both success and edge cases
w.Write([]byte("hello\n"))
w.Write([]byte("world")) // Test without EOL
This approach makes tests more maintainable, easier to debug, and less prone to flaky failures.
Maintain strict security boundaries in CI/CD workflows by treating CI as a validation-only system and carefully managing third-party dependencies. CI should never automatically modify contributor code or commits - its role is to verify that everything is correct, not to replace proper development practices.
For third-party GitHub Actions, especially those that are less trusted, pin them to specific commit hashes rather than version tags to prevent supply chain attacks. When upgrading actions, carefully review changes and be aware of new default behaviors that might have security implications.
Example of secure action pinning:
# Instead of:
uses: tibdex/github-app-token@v2
# Use:
uses: tibdex/github-app-token@v2.1.0 # or pin to commit hash
Be particularly cautious of actions that enable features like provenance generation by default, as these may upload artifacts or require additional permissions. Always explicitly configure such features rather than relying on defaults, and add verification steps to ensure the security chain is complete.
Maintain consistency with established naming conventions throughout the codebase. When adding new flags, functions, or variables, examine existing similar implementations and follow the same patterns for naming, separators, and structure.
Key areas to check:
--timestamps not --timestamp to match compose logs)GetImageNameOrDefault instead of GetDefaultImageName when reading existing values first)- for resource names, : for label formatting)streams not dockerCli when type is api.Streams)Example from codebase:
// Consistent with existing logs command
flags.BoolVar(&up.timestamps, "timestamps", false, "Show timestamps.")
// Descriptive function name that matches behavior
func GetImageNameOrDefault(service types.ServiceConfig, projectName string) string {
if service.Image != "" {
return service.Image
}
return projectName + "-" + service.Name // Use consistent separator
}
Before introducing new naming, search the codebase for similar functionality and adopt the established patterns to maintain consistency and reduce cognitive load for developers.
When implementing health checks, use comprehensive condition evaluation patterns instead of simple field comparisons. Check multiple condition attributes (type, status, reason, message) to ensure accurate health status determination.
Simple field checks can miss important state information and lead to incorrect health assessments. Instead of checking a single ready field, evaluate the full condition structure to get a complete picture of resource health.
Example of preferred approach:
if obj.status ~= nil and obj.status.conditions ~= nil then
for _, condition in ipairs(obj.status.conditions) do
if condition.type == "Ready" and condition.status == "True" and condition.reason == "Succeeded" then
hs.status = "Healthy"
return hs
end
end
end
Avoid oversimplified checks like if obj.status.ready == "true" or if obj.status.ready then when structured conditions are available. This pattern improves observability by providing more reliable health status reporting and better debugging information when issues occur.
When documenting network-dependent features, explicitly specify all network configuration requirements including protocol support, certificate handling, and infrastructure setup. Users often encounter connectivity issues due to undocumented network prerequisites.
Include specific configuration examples and verification steps for:
Example documentation pattern:
# Network Requirements
!!! note
This feature requires HTTP/2 support. Verify your infrastructure supports HTTP/2.
# TLS Configuration (for self-signed certificates)
spec:
# Skip TLS validation for self-signed certificates
insecure: true
# Reference to trusted CA certificates
caRef:
configMapName: argocd-tls-certs-cm
key: azure-devops-ca
# Ingress Configuration
apiVersion: networking.k8s.io/v1
kind: Ingress
spec:
rules:
- host: cd.apps.argoproj.io
http:
paths:
- path: /api/webhook
pathType: Prefix
backend:
service:
name: argocd-applicationset-controller
port:
number: 7000
This prevents user frustration from undocumented network dependencies and reduces support burden from connectivity-related issues.
Configure different optimization parameters for different resource types (CPU, memory, GPU) rather than applying uniform settings across all resources. Different resources have distinct characteristics and usage patterns that require tailored optimization strategies for maximum performance and utilization.
For example, when implementing overcommit functionality, break down generic factors into resource-specific components:
- plugins:
- name: overcommit
arguments:
cpu-overcommit-factor: 1.2
mem-overcommit-factor: 1.0
other-overcommit-factor: 1.2
Similarly, for GPU scheduling, consider CPU, memory, and GPU memory together with different weights and strategies:
- plugins:
- name: binpack
arguments:
binpack.cpu: 5
binpack.memory: 1
binpack.resources.nvidia.com/gpu: 2
This approach prevents suboptimal resource allocation that occurs when treating all resources uniformly, such as over-provisioning incompressible resources or under-utilizing resources with different performance characteristics. Always analyze the specific behavior and constraints of each resource type before applying optimization parameters.
Choose names that are self-explanatory, descriptive, and consistent across the codebase. Avoid abbreviated or informal naming that requires additional explanation. Follow language-specific conventions and include units or data types where appropriate.
Key principles:
maximum-runtime instead of runsec-max)ReserveNodesMap instead of nodeForbidMap)maxRetry consistently rather than mixing jobRetry and maxRetry)500s instead of just 500)reserveable: true instead of is-reserve: 1)Example improvements:
# Before - informal and unclear
volcano.sh/is-reserve: 1
volcano.sh/runsec-max: 500
# After - descriptive and clear
volcano.sh/reserveable: true
volcano.sh/maximum-runtime: 500s
// Before - requires explanation
nodeForbidMap map[string]bool
jobRetry int
// After - self-explanatory and consistent
ReserveNodesMap map[string]bool
maxRetry int
When designing configuration management systems, avoid strong dependencies on external projects for configuration logic and data structures. External projects may not evolve at the same pace as your codebase, creating maintenance burdens and potential compatibility issues.
Instead of relying on external configuration frameworks, internalize critical configuration components within your project. This includes rewriting essential configuration handlers, defining custom resource definitions (CRDs) for complex configuration data, and consolidating scattered configuration annotations into structured formats.
For example, rather than using multiple annotation entries for configuration state:
# Avoid: Multiple scattered annotations from external projects
annotations:
nos.io/mig-profile-1: "free"
nos.io/mig-profile-2: "used"
nos.io/pod-request-1: "profile-x"
Consolidate into a single, well-structured configuration:
# Prefer: Single consolidated configuration structure
annotations:
volcano.sh/gpu-config: |
{
"migProfiles": [
{"name": "profile-1", "status": "free"},
{"name": "profile-2", "status": "used"}
],
"requests": [{"profile": "profile-x"}]
}
This approach ensures configuration evolution remains under your control and reduces external dependency risks.
Ensure configuration behavior is predictable, validated, and migration-friendly:
get() patterns when the option is already defined).ImproperlyConfigured rather than using silent fallbacks). If a setting is expected to be callable/type-specific, check it.Example (fail-fast validation for a config-provided function):
from celery.exceptions import ImproperlyConfigured
repr_function = self.app.conf.task_args_repr_function
if not repr_function:
raise ImproperlyConfigured("task_args_repr_function must be set")
if isinstance(repr_function, str):
# resolve symbol_by_name(...)
repr_function = symbol_by_name(repr_function)
if not callable(repr_function):
raise ImproperlyConfigured("task_args_repr_function must be callable")
Example (explicit precedence):
broker_url or CELERY_BROKER_*_URL wins, implement that order consistently, and ensure it’s documented as a behavior contract.Always verify that properties are not null or undefined before calling methods on them to prevent runtime errors. This is especially important when filtering, mapping, or processing arrays where some objects may have missing or undefined properties.
When working with potentially null properties, add explicit null checks before accessing methods:
// Unsafe - can throw if project is null/undefined
const newRepos = repos.filter(repo => repo.project.includes(project));
// Safe - checks for null/undefined first
const newRepos = repos.filter(repo => repo.project && repo.project.includes(project));
// For arrays, filter out null/undefined values before processing
const projectItems = projectValues
.filter(project => project && project.trim() !== '')
.map(project => ({
title: project,
action: () => this.setState({ projectProperty: project })
}));
This pattern prevents “Cannot read property ‘X’ of null/undefined” errors and makes code more robust when dealing with optional or potentially missing data.
Make all configuration settings explicit and well-documented, avoiding implicit defaults and auto-detection where possible. This includes dependency versions, build flags, environment variables, and tool configurations.
Key principles:
GO111MODULE=on, CGO_ENABLED=0)Example of explicit configuration in Makefile:
# Explicit module and CGO settings
GO111MODULE=on
CGO_ENABLED=0
# Simple default with override capability
BUILDX_CMD ?= docker buildx
# Clear documentation for temporary workarounds
replace (
// reverts https://github.com/moby/buildkit/pull/4094 to fix fsutil issues on Windows
github.com/docker/buildx => github.com/crazy-max/buildx v0.8.1-0.20240130141015-d7042ae5516c
)
This approach reduces surprises, makes builds more reproducible, and helps maintainers understand the reasoning behind configuration choices.
Scripts that depend on environment variables should validate their presence and values early, providing clear error messages that guide users toward resolution. When checking environment variables, ensure logical flow doesn’t create unreachable code blocks.
For boolean environment variables, validate the expected values and provide actionable feedback:
# Good: Clear validation with actionable error message
if [ "$ARGOCD_REDIS_LOCAL" = 'true' ]; then
if ! command -v redis-server &>/dev/null; then
echo "Redis server is not installed locally. Please install Redis or set ARGOCD_REDIS_LOCAL to false."
exit 1
fi
# Continue with local Redis setup...
fi
# Avoid: Logic that makes subsequent code unreachable
if [ "$ARGOCD_REDIS_LOCAL" = 'true' ] && ! command -v redis-server &>/dev/null; then
echo "Redis server not found"
exit 1 # This prevents local Redis startup code from running
fi
Always structure conditional logic to ensure all valid configuration paths remain accessible and provide specific guidance on how to resolve configuration issues.
Avoid duplicating RBAC permissions across different deployment configurations or installation modes. When the same service or controller needs permissions in multiple contexts (e.g., cluster-wide vs namespace-scoped deployments), consolidate these permissions into a unified, well-defined role structure rather than maintaining separate, potentially inconsistent permission sets.
This practice ensures:
For example, instead of maintaining separate cluster roles and namespace roles with overlapping permissions, create a comprehensive role definition that can be appropriately scoped based on the deployment context:
# Consolidated approach - single source of truth for permissions
rules:
- apiGroups: [""]
resources: ["configmaps"]
verbs: ["create", "update"]
- apiGroups: ["coordination.k8s.io"]
resources: ["leases"]
verbs: ["create", "update"]
Review existing RBAC configurations to identify and eliminate permission duplication, ensuring that authorization controls remain consistent and maintainable across different deployment scenarios.
Ensure all user-configurable options are properly documented and discoverable, while keeping configuration files clean and focused. Configuration files serve as APIs that users rely on to understand available options.
Key principles:
values.yaml with clear documentationExample from Helm values:
# values.yaml
global:
# Labels to apply to all resources
# Set this if you need custom labels on all chart resources
customLabels: {}
# Internal usage only - controls API version selection
# Users should not modify this directly
autoscalingv2API: true
# Avoid unnecessary defaults like:
# test_pod:
# enabled: false # Don't include if not needed
# image: bats/bats:v1.1.0 # Don't include if disabled
This ensures users can easily discover configuration options while maintaining clean, focused configuration files that don’t overwhelm with unnecessary details.
Configuration values should provide sensible defaults while remaining adaptable to different environments and deployment contexts. Use conditional assignment and guards to handle variations in runtime environments, deployment tools, and system configurations.
For build configurations, use conditional assignment to allow environment-specific overrides:
# Allow override for different systems (e.g., NixOS)
SHELL ?= /bin/bash
For template configurations, add guards around optional values that may not be available in all contexts:
{{- define "istio-labels" }}
app.kubernetes.io/name: ztunnel
{{- if .Release.Service }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}
{{- end }}
This approach ensures configurations work reliably across different deployment methods (helm install, helm template, istioctl) and system environments without requiring manual intervention or causing failures when optional values are missing.
Prefer structured logging frameworks like logrus over direct fmt output functions for consistency, proper log levels, and better log management. This ensures uniform logging behavior across the codebase and enables proper log level filtering and formatting.
Use logrus methods with appropriate log levels instead of fmt.Printf, fmt.Fprintf, or fmt.Fprintln for application messages:
// Instead of:
fmt.Fprintf(s.stderr(), "Failed to parse aux message: %s", err)
_, _ = fmt.Fprintln(out, "Terminal is not a POSIX console")
// Use:
logrus.Errorf("Failed to parse aux message: %s", err)
logrus.Warning("Terminal is not a POSIX console")
Choose the appropriate log level (Debug, Info, Warn, Error, Fatal) based on the message severity. Reserve fmt functions only for direct user output or when specifically required to write to particular streams.
Maintain code readability by minimizing nesting depth and using appropriate control structures. Prefer flat code organization over deeply nested conditions. When dealing with multiple conditions:
Example - Instead of:
if condition {
if nestedCondition {
for name, s := range items {
if deeplyNested {
// actual logic
}
}
}
}
Prefer:
if !condition {
return
}
if !nestedCondition {
return
}
switch {
case condition1:
// handle case
case condition2:
// handle case
default:
// handle default
}
This approach improves code readability, reduces cognitive load, and makes the code easier to maintain and debug.
Ensure configuration documentation clearly and accurately describes environment variables, command-line flags, and their relationships. Pay special attention to precedence rules, equivalent options, and proper grammar when explaining configuration behavior.
Key areas to review:
Example of good documentation:
# Clear precedence explanation
Setting the `COMPOSE_PROJECT_NAME` environment variable is equivalent to the `-p` flag,
and `COMPOSE_PROFILES` environment variable is equivalent to the `--profiles` flag.
If a flag is set via the command line, the associated environment variable is ignored.
This ensures developers understand exactly how different configuration methods interact and which takes precedence in various scenarios.
Celery should only execute tasks when its assumptions about state and message format are guaranteed.
Apply these rules:
1) Django: trigger tasks after the DB commit
send_email.delay(...) (or similar) inside the request flow before the transaction is committed.transaction.on_commit(...) or Celery’s Django task integration.Example (safe timing):
# views.py
from django.db import transaction
def create_user(request):
user = User.objects.create(username=request.POST['username'])
transaction.on_commit(lambda: send_email.delay(user.pk))
return HttpResponse('User created')
Optional Celery shortcut (Django task class):
from celery import Celery
app = Celery('proj', task_cls='celery.contrib.django.task:Task')
2) Kafka (and other transports): configure required serializers precisely
result_serializer unless you actually need to.Example:
# celeryconfig.py
task_serializer = 'json' # required
# result_serializer = ... # only set if you have a need
Maintain consistent formatting conventions across log messages to improve code readability and debugging effectiveness. Use proper format specifiers (%v for errors, not %s), maintain consistent spacing around punctuation (no spaces before colons), and ensure consistent label semantics when logging similar data structures.
Key formatting guidelines:
%v format specifier for error values instead of %sExample of good formatting:
// Good - consistent formatting, proper specifier
log.Infof("connection closed: %v", err)
log.WithLabels("removes", len(resp.RemovedResources)).Debugf("forward ECDS")
// Bad - inconsistent spacing, wrong specifier, redundant text
log.Infof("connection is not closed with error : %s", err)
log.WithLabels("removals", resp.RemovedResources).Debugf("forward ECDS")
This consistency helps maintain professional code quality and makes log analysis more predictable for debugging and monitoring.
Design API methods using options objects instead of multiple parameters. This pattern provides better extensibility, version compatibility, and maintains consistent interfaces across the codebase.
Benefits:
Example:
// Instead of this:
func (s *Service) Publish(ctx context.Context, project *types.Project, repository string) error {
// ...
}
// Do this:
type PublishOptions struct {
Project *types.Project
Repository string
Quiet bool // Future-proof for additional options
}
func (s *Service) Publish(ctx context.Context, opts PublishOptions) error {
// Version-specific feature check example
if opts.NewFeature != nil {
version, err := s.RuntimeVersion(ctx)
if err != nil {
return err
}
if versions.LessThan(version, "1.44") {
return errors.New("feature requires Docker Engine 1.44 or later")
}
}
// ...
}
Ensure that logging configuration options actually perform the behavior their names suggest and provide real value. Before implementing or maintaining logging features, verify that they work as advertised and aren’t just cosmetic labels.
This addresses cases where logging configurations appear functional but don’t actually control system behavior. For example, a log_sampled field that sets a boolean flag but doesn’t actually sample requests, or logging policies that exist in code but have no operational impact.
When reviewing logging configurations:
Example of problematic configuration:
{
"log_sampled": "true" // Misleading - doesn't actually sample, just sets a label
}
Rather than maintaining misleading features, either implement proper functionality or remove the configuration entirely. This prevents wasted development effort and user confusion about system behavior.
When modifying collections during iteration, use safe patterns to avoid subtle bugs and undefined behavior. Common unsafe patterns include modifying map values during range iteration, capturing loop variables by reference, and deleting from slices while iterating.
Safe Patterns:
// Instead of modifying during iteration:
for name, service := range set {
service.Scale++ // Unsafe if service is a value
set[name] = service // Update map after modification
}
// Safe pattern - explicit assignment:
for k, v := range source {
v := v // Create new variable to avoid pointer capture
dest[k] = &v
}
// Instead of deleting during forward iteration:
// Create new slice and copy elements to keep
newVolumes := make([]Volume, 0, len(s.Volumes))
for _, vol := range s.Volumes {
if shouldKeep(vol) {
newVolumes = append(newVolumes, vol)
}
}
s.Volumes = newVolumes
These patterns prevent common algorithmic errors where collection state changes during iteration lead to skipped elements, incorrect references, or unpredictable behavior.
Before adding or updating dependencies, thoroughly evaluate API compatibility and understand the implications of using forks versus upstream versions. This prevents recurring dependency issues and ensures long-term maintainability.
Key evaluation steps:
Example from dependency analysis:
// Before adding:
require github.com/tilt-dev/fsnotify v1.4.8-0.20220602155310-fff9c274a375
// Evaluate: What API differences exist?
// - SetRecursive method not in upstream
// - Windows-specific patches upstreamed but not released
// - Consider pinning to upstream commit hash as interim solution
This approach helps avoid situations where dependencies cause recurring problems or create maintenance burdens due to API incompatibilities.
Always pin specific versions for build dependencies and tools to prevent unexpected build failures, even when the dependency won’t be shipped in the final product. While rolling tags may be convenient, explicit version pinning provides build reproducibility and prevents surprises from automatic updates.
For build tools that don’t ship with the final product, pin to specific versions to avoid sudden build breaks:
# Pin specific versions for build stability
ARG GO_VERSION=1.21.0
ARG PYTHON_VERSION=3.7.9
# FIXME: virtualenv 16.3.0 breaks build, force 16.2.0 until fixed
RUN pip install virtualenv==16.2.0 tox==2.1.1
This approach is especially important for build tools and dependencies that are used during the build process but not included in the final image. Even if patch releases are generally safe, pinning versions ensures consistent builds across different environments and prevents unexpected failures when dependencies are automatically updated.
Always validate potentially null or empty values before use, and provide explicit default values when handling optional data. This prevents runtime errors and makes code behavior more predictable.
Key practices:
Example:
// Before
func processValue(m map[string]interface{}, key string) string {
return m[key].(string)
}
// After
func processValue(m map[string]interface{}, key string) string {
if v, ok := m[key].(string); ok && v != "" {
return v
}
return "" // explicit default
}
// Even better with generics
func valueOrDefault[T any](m map[string]interface{}, key string) T {
if v, ok := m[key].(T); ok {
return v
}
return *new(T)
}
Always wrap errors with meaningful context and use typed error checking instead of string matching. This helps with error tracing and maintains consistent error handling patterns.
Key practices:
errors.Wrap() or fmt.Errorf() with %w to preserve error chainserrdefs.IsNotFound()Example:
// Bad:
if err != nil && strings.HasPrefix(err.Error(), "no container found") {
return nil
}
// Good:
if err != nil {
if errdefs.IsNotFound(err) {
return nil
}
return fmt.Errorf("failed to create volume %q: %w", name, err)
}
// Bad:
return fmt.Errorf("volume %q exists but not created by Compose", name)
// Good:
return fmt.Errorf("volume %q already exists but was not created by Docker Compose. Use `external: true` to use an existing volume", name)
When documenting CI/CD processes, build pipelines, or deployment workflows, use clear and precise language that helps developers understand the sequence of operations. Write in present tense to describe what tools do, and provide explicit references when explaining examples.
Key practices:
Example of clear CI/CD documentation:
### Use Dry Run mode to test your command
Use `--dry-run` flag to test a command without changing your application stack state.
Dry Run mode shows you all the steps Compose applies when executing a command, for example:
```console
$ docker compose --dry-run up --build -d
[+] Running 10/8
✔ DRY-RUN MODE - Container nginx-golang-mysql-db-1 Created
✔ DRY-RUN MODE - Container nginx-golang-mysql-backend-1 Created
From the example above, you can see that the first step is to pull the image defined by db service, then build the backend service.
This approach ensures that team members can easily follow and understand CI/CD processes, reducing confusion during deployment and build operations.
---
## Prevent unintended CI behaviors
<!-- source: docker/compose | topic: CI/CD | language: Go | updated: 2023-04-28 -->
Ensure CI/CD pipelines maintain predictable and reliable behavior by avoiding changes that introduce unexpected side effects or alter default behaviors. This includes preventing automatic actions like image pushing when not explicitly requested, using consistent tooling approaches across similar operations, and properly handling unreliable components.
Key practices:
- Preserve default behaviors unless explicitly overridden by user intent
- Use consistent tooling approaches (e.g., if using vendored clients for standard builds, use them for related operations too)
- Skip or isolate flaky tests in CI environments rather than letting them destabilize the pipeline
- Add comprehensive verification to ensure expected behaviors
---
## Document observability clearly
<!-- source: istio/istio | topic: Observability | language: Markdown | updated: 2023-03-21 -->
Ensure observability documentation is clear, accessible, and grammatically correct. This includes defining technical acronyms and ensuring proper sentence structure in configuration explanations.
When documenting observability systems, always:
- Define technical acronyms on first use with links to official documentation when available
- Use proper grammar and clear sentence structure in configuration explanations
- Ensure warnings and notes about system behavior are unambiguous
Example of good practice:
```markdown
# Open Telemetry ALS with Loki
This sample demonstrates Access Log Service (ALS) integration with Loki for distributed tracing.
> **Warning**
> When the example `PodMonitor` is used with OpenShift Monitoring, it must be created in all namespaces where istio-proxies exist.
> This is because `namespaceSelector` is ignored for tenancy isolation.
Clear documentation prevents misconfigurations and reduces support overhead in observability deployments.
When handling security credentials (particularly TLS certificates) in data structures intended for transmission or serialization, include only the minimum necessary identifying information. Avoid passing complete certificate chains or redundant fields that have already been validated upstream.
Instead of including entire certificate structures with all their fields:
type AuthInfo struct {
// Don't include these large fields
// TLSConnection.State.PeerCertificates []x509.Certificate
// TLSConnection.State.VerifiedChains [][]x509.Certificate
// Instead, extract only essential identifying information
FQDN string `json:"fqdn,omitempty"`
Fingerprint string `json:"fingerprint,omitempty"`
CommonName string `json:"commonName,omitempty"`
}
This approach reduces payload size, improves performance, and follows the principle of least exposure for sensitive security information. When designing security-related data structures, always ask whether each field is truly necessary for the downstream consumer.
When implementing eBPF networking programs, prefer using kernel-native data structures and BPF helpers over maintaining custom eBPF maps that duplicate network routing information. The kernel already tracks IP-to-MAC mappings, interface information, and routing data through its networking stack.
Instead of creating and maintaining parallel eBPF maps with pod IP addresses, interface indices, and MAC addresses, use BPF helpers like bpf_fib_lookup() to dynamically query the kernel’s existing network state. This approach offers several advantages:
Example of the preferred approach:
// Instead of maintaining custom pod routing maps:
// struct { ... } app_info SEC(".maps");
// Use kernel helpers for dynamic lookup:
SEC("tc")
int network_redirect(struct __sk_buff *skb) {
// Use bpf_fib_lookup to get device/MAC from dest IP
struct bpf_fib_lookup fib_params = {};
// ... populate fib_params from packet data
int ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
// Use fib_params.ifindex and fib_params.dmac
return bpf_redirect(fib_params.ifindex, 0);
}
}
This principle applies especially when the custom eBPF map would contain information that’s already available through kernel networking APIs and BPF helpers.
Always verify the licensing compatibility of all dependencies, especially in security-critical networking code. GPL and other restrictive licenses can create legal compliance issues and security vulnerabilities through forced disclosure requirements or usage restrictions.
Before integrating external libraries, headers, or frameworks:
Example from BPF networking code:
// Problematic - GPL-only dependencies
#include <linux/bpf.h> // GPL license
#include <bpf/bpf_helpers.h> // GPL license
// Solution - Use dual-licensed alternatives or
// implement compatible functionality
char __license[] __section("license") = "Dual BSD/GPL";
License restrictions can compromise security by limiting your ability to patch vulnerabilities, distribute security updates, or maintain code independently. Always audit licensing before committing to dependencies in security-sensitive components.
Ensure telemetry attribute population logic is consistent across all components and follows established standards. When implementing telemetry features like resource attributes, baggage headers, or tracing configurations, use the same business logic and attribute naming conventions throughout the codebase.
Key requirements:
Example of consistent attribute population:
// Use the same logic across components
func resourceAttributes(proxy *Proxy) *otlpcommon.KeyValueList {
return &otlpcommon.KeyValueList{
Values: []*otlpcommon.KeyValue{
// Use OpenTelemetry semantic convention constants
otelKeyValue(otelsemconv.K8SClusterNameKey, proxy.Metadata.ClusterID.String()),
otelKeyValue(otelsemconv.K8SNamespaceNameKey, proxy.ConfigNamespace),
otelKeyValue(otelsemconv.K8SPodNameKey, podName),
otelKeyValue(otelsemconv.ServiceNameKey, proxy.XdsNode.Cluster),
},
}
}
This approach prevents inconsistencies that can lead to fragmented telemetry data and ensures compliance with industry standards, making the observability system more reliable and interoperable.
Identify and eliminate expensive operations in frequently executed code paths, particularly in packet processing and network handling contexts. Hot paths are code sections that execute repeatedly at high frequency, where even small performance penalties can significantly impact overall system performance.
Common expensive operations to avoid in hot paths include:
Instead, use performance-optimized alternatives:
Example from eBPF packet processing:
// Avoid: expensive lookup per packet
sk = bpf_skc_lookup_tcp(skb, tuple, tuple_len, BPF_F_CURRENT_NETNS, 0);
// Better: use map-based lookup
struct connection_info *conn = map_lookup_elem(&connection_map, &key);
// Avoid: debug logging in production hot path
#ifdef DEBUG_MODE
printk("Processing packet from %pI4\n", &iph->saddr);
#endif
Profile and measure hot paths to identify bottlenecks, then prioritize optimizing the most frequently executed expensive operations first.
When selecting API versions, prioritize compatibility over using the latest available version. Choose older, stable API versions when they provide the same schema but broader compatibility across Kubernetes versions. This approach minimizes breaking changes for users and reduces deployment friction.
Consider deprecation timelines when making version choices - select versions that give users adequate migration time. For example, prefer autoscaling/v2beta2 over autoscaling/v2beta1 when both work, since v2beta2 will be supported longer.
Use conditional logic in templates to handle version selection based on Kubernetes capabilities:
{{- if (semverCompare ">=1.23-0" .Capabilities.KubeVersion.GitVersion)}}
apiVersion: autoscaling/v2
{{- else }}
apiVersion: autoscaling/v2beta2
{{- end }}
The principle is: “use the oldest version everywhere (it’s strictly better - same schema but more compatible, and requires no noise from changing a bunch of files)”. Be conservative with API version updates, especially in configurations that directly impact users.
Ensure consistent terminology, precise language, and uniform formatting throughout all documentation to improve clarity and user experience. Use standardized terms consistently (e.g., “V1” and “V2” not “v1” and “v2”), choose precise wording that clearly conveys intent, and maintain consistent formatting patterns.
Key practices:
Example of consistent terminology:
# Good
Pull requests must be cleanly rebased on top of the base branch without multiple branches
# Avoid
Pull requests must be cleanly rebased on top of master without multiple branches
Example of precise language:
# Good
4. Include code comments. Tell us the why, the history and the context.
# Avoid
4. Comment on the code. Tell us the why, the history and the context.
This consistency reduces confusion, improves maintainability, and creates a more professional user experience across all documentation.
Configuration values should be parameterized rather than hardcoded in scripts and tools. Hardcoded values make code less reusable and require manual updates when used in different contexts or projects.
Instead of embedding project-specific strings directly in code, use variables, parameters, or configuration files. This applies to project names, paths, environment variables, and other context-specific values.
Example of problematic hardcoded configuration:
if [[ "${k}" == "github.com/containerd/containerd"* ]]; then
continue
fi
Better approach - parameterize the value:
if [[ "${k}" == "github.com/docker/compose"* ]]; then
continue
fi
For environment variables and tool wrappers, ensure consistent behavior across different execution contexts. When wrapping tools, maintain the same environment variable handling as the original tool while avoiding hardcoded assumptions about paths or configurations.
This practice improves code reusability, reduces maintenance overhead, and prevents configuration-related bugs when code is used in different environments or projects.
Keep test dependencies separate from production code dependencies to avoid polluting the main module. Use separate modules for test code when test-specific dependencies would otherwise be included in the main module’s dependency tree.
This prevents test libraries from becoming transitive dependencies for consumers of your module and maintains a cleaner separation between production and test concerns.
Example approach:
// Create separate go.mod for e2e tests
// e2e/go.mod
module github.com/docker/compose/v2/e2e
require (
github.com/cucumber/godog v0.12.5
github.com/docker/compose/v2 v0.0.0
)
replace github.com/docker/compose/v2 => ../
This pattern allows test-specific dependencies like testing frameworks, mocking libraries, or specialized test utilities to remain isolated from the main codebase while still allowing tests to import and test the production code.
Avoid pickle unless you have a documented, justified need and you can guarantee the serialized data never comes from untrusted sources. Prefer safe, data-only formats (e.g., JSON) for serialization/deserialization in tests and production code.
Example (safer alternative pattern):
import json
payload = {
"app": "sentinel-app",
"max_interval": "sentinel-max_interval",
"schedule_filename": "sentinel-schedule_filename",
"scheduler_cls": "sentinel-scheduler_cls",
}
# Round-trip the data representation (no code execution during parsing)
serialized = json.dumps(payload)
restored = json.loads(serialized)
If pickle is unavoidable, require a clear justification in code/comments and ensure it is only ever used with objects/data produced locally (no user input, no external storage, no network-delivered blobs).
Choose variable names that prevent conflicts with system variables, external tools, and variables in overlapping scopes. Use prefixes or distinct naming to differentiate purpose and avoid unintended overrides.
For system/tool conflicts, use prefixed names:
# Instead of LDFLAGS (conflicts with rpm/deb packaging)
GO_LDFLAGS ?= -s -w -X ${PKG}/internal.Version=${VERSION}
For same-scope conflicts, use different names for different purposes:
# Instead of same name for ARG and ENV
ARG GIT_COMMIT=unknown
ENV DOCKER_COMPOSE_GITSHA=$GIT_COMMIT
This prevents situations where variables are unexpectedly overridden by external tools or conflicting declarations in the same context.
When working with GitHub Actions workflows that require shared credentials or tokens, leverage organizational-level secrets instead of duplicating them across individual repositories. This approach centralizes secret management, reduces the attack surface, and simplifies maintenance.
Organizational secrets should be used for credentials that are shared across multiple repositories within the same organization, such as deployment tokens, API keys for shared services, or documentation dispatch tokens.
Example from a GitHub Actions workflow:
steps:
- name: Sending event to upstream repository
uses: actions/github-script@v6
with:
github-token: ${{ secrets.GHPAT_DOCS_DISPATCH }}
In this case, GHPAT_DOCS_DISPATCH should be configured at the organization level rather than being added to each individual repository that needs it. This ensures consistent access control, easier rotation of credentials, and prevents secret sprawl across repositories.
Design CI workflows to prevent resource conflicts when multiple builds run concurrently on shared infrastructure. This includes using unique identifiers for temporary resources, minimizing container privileges, and separating CI-specific operations from local development workflows.
Key practices:
Example of problematic code:
TMP_CONTAINER="tmpcontainer" # Fixed name causes conflicts
docker run --privileged ... # Unnecessary privileges
Better approach:
TMP_CONTAINER="tmpcontainer-$(date +%s)-$$" # Unique per run
docker run ... # Only add --privileged if actually needed
This prevents build failures when multiple PRs or builds execute simultaneously on shared CI infrastructure, and ensures local development workflows remain usable without requiring system-level permissions.
Implement specific exception handling and defensive programming practices to prevent generic errors from masking real issues and provide graceful degradation when operations fail.
Key principles:
Exception to avoid hiding legitimate errorsExample of problematic generic exception handling:
try:
ctnr.kill()
except Exception: # Too broad - hides real issues
pass
Better approach with specific exception handling:
try:
ctnr.kill()
except docker.errors.APIError: # Specific to expected failure
log.warning(f"Failed to kill container {ctnr.name}")
Example of defensive programming:
# Check before accessing to prevent IndexError
containers = project.up(service_names=[service.name])
matching_containers = [c for c in containers if c.service == service.name]
if not matching_containers:
raise OperationFailedError("Could not bring up the requested service")
container = matching_containers[0]
This approach ensures errors are handled appropriately without masking underlying issues, while providing users with meaningful feedback when operations fail.
When modifying configuration schemas, always propose changes to the upstream/source repository first before implementing them locally. This practice prevents schema divergence, maintains version integrity, and ensures a single source of truth for configuration definitions.
Key principles:
Example from the discussions:
// Instead of directly modifying local schema files
"secrets": {"$ref": "#/definitions/build_secrets"}
// First propose the change to upstream compose-spec repository
// Then reference the updated upstream schema
This approach ensures configuration schemas remain consistent across the ecosystem and prevents the fragmentation that occurs when teams make isolated changes to shared configuration standards.
Choose explicit, readable code constructs over “clever” or overly complex ones. Complex functional programming constructs, intricate ternary operators, and large list comprehensions can make code harder to understand and maintain.
Prefer explicit conditionals:
# Instead of:
msg = not silent and 'Pulling' or None
# Use:
msg = 'Pulling' if not silent else None
Avoid complex functional constructs when simpler alternatives exist:
# Instead of reduce() with mixed return types:
return reduce(combine_configs, configs + [None])
# Use explicit loops:
result = None
for config in configs:
result = combine_configs(result, config)
return result
Break down complex list comprehensions:
# Instead of nested, multi-line list comprehensions:
service_names = [
service.name
for service in self.services
if 'profiles' not in service.options or [
e for e in service.options.get('profiles')
if e in enabled_profiles
]
]
# Extract to a clear method:
service_names = [
service.name
for service in self.services
if service.enabled_for_profiles(enabled_profiles)
]
This approach makes code easier to debug, test, and understand for team members, reducing cognitive load during code reviews and maintenance.
Never use mutable objects (lists, dictionaries, sets) as default parameter values in function definitions. This creates a shared object across all function calls, leading to unexpected behavior where modifications persist between calls.
Instead, use None as the default value and assign the mutable object inside the function body. This ensures each function call gets a fresh instance of the mutable object.
Problem:
def get_project(project_dir, config_path=None, enabled_profiles=[]):
# enabled_profiles is shared across all calls!
enabled_profiles.append("default")
return enabled_profiles
def project_from_options(project_dir, options, additional_options={}):
# additional_options is shared across all calls!
additional_options["key"] = "value"
return additional_options
Solution:
def get_project(project_dir, config_path=None, enabled_profiles=None):
enabled_profiles = enabled_profiles or []
enabled_profiles.append("default")
return enabled_profiles
def project_from_options(project_dir, options, additional_options=None):
additional_options = additional_options or {}
additional_options["key"] = "value"
return additional_options
This pattern prevents the “least astonishment” principle violation where the same mutable object is reused across function invocations, causing side effects that can be difficult to debug.
Choose clear, unambiguous names that don’t shadow existing identifiers or create confusion about their purpose. Avoid variable names that shadow method names, class names, or built-in functions, as this makes code harder to understand and can suggest incorrect behavior like recursive calls.
When naming variables, methods, or writing documentation text, prioritize clarity over brevity. If a name could be interpreted multiple ways or conflicts with existing identifiers, choose a more descriptive alternative.
Example of problematic naming:
# Bad: 'build' shadows the method name, suggesting recursion
build = self.client.build if not _exec else exec_build
build_output = build(...)
Example of clear naming:
# Good: Clear intent, no shadowing
build_func = self.client.build if not _exec else exec_build
build_output = build_func(...)
This principle also applies to documentation and help text - avoid ambiguous product names or terminology that could confuse users about what they’re actually using.
When using git references for dependencies in configuration files like requirements.txt, always pin to specific commit SHAs instead of branch names to ensure reproducible builds and prevent unexpected changes from being automatically pulled in.
Using branch names like master or main can lead to non-deterministic builds where different developers or deployment environments might pull different versions of the code, potentially introducing breaking changes or inconsistent behavior.
Example of what to avoid:
git+https://github.com/docker/docker-py.git@master#egg=docker-py
Example of proper pinning:
git+https://github.com/docker/docker-py.git@a1b2c3d4e5f6789012345678901234567890abcd#egg=docker-py
This practice ensures that all team members and deployment environments use exactly the same version of the dependency, making builds more predictable and debugging easier when issues arise.
When adding logging functionality to existing code, encapsulate the logging logic within methods or use parameters to avoid code duplication and maintain clean interfaces. Add informational logging (debug/info level) to provide visibility into system operations, especially for default behaviors and skipped actions, but avoid duplicating conditional checks when the underlying methods already handle them.
For example, instead of duplicating logic across multiple code paths:
# Bad - duplicated logging logic
if container.has_api_logs and not detached:
container.attach_log_stream()
# ... repeated in multiple places
# Good - encapsulate in method parameters
self.start_container_if_stopped(container, attach_logs=not detached)
Also add debug logging for default behaviors that might not be obvious:
# Good - inform about default file selection
log.debug("Using default config file: %s", winner)
Always use os.environ.get() instead of direct dictionary access when reading environment variables to avoid KeyError exceptions. Establish clear precedence rules where command line options override environment variables, and log conflicts appropriately when incompatible configuration sources are detected.
When accessing environment variables, prefer the safe getter method:
# Good
profiles = environment.get('COMPOSE_PROFILE')
ignore_orphans = environment.get_boolean('COMPOSE_IGNORE_ORPHANS')
# Avoid
if 'CLICOLOR' in os.environ and os.environ['CLICOLOR'] == "0"
For configuration precedence, implement command line options taking priority over environment variables:
def compatibility_from_options(working_dir, options=None, environment=None):
# Command line takes precedence over environment
if options and options.get('--compatibility'):
return True
if environment and 'COMPOSE_COMPATIBILITY' in environment:
return environment.get_boolean('COMPOSE_COMPATIBILITY')
return False
When conflicting configuration is detected, log the conflict clearly:
if ignore_orphans and options['--remove-orphans']:
log.warn("COMPOSE_IGNORE_ORPHANS is set, --remove-orphans flag is being ignored.")
This approach prevents runtime errors from missing environment variables and ensures predictable configuration behavior across different deployment environments.
When designing APIs or interfaces, prioritize using existing standard fields and patterns from established APIs rather than creating custom implementations. This ensures consistency across tools and reduces cognitive load for users.
Key principles:
Example from Docker Compose:
# Instead of creating custom status formatting
container.human_readable_state
# Use the standard Status field from Docker API
container.status # Returns "Up 5 minutes" format used by docker CLI
This approach improves user experience through familiarity, reduces maintenance burden by leveraging tested implementations, and ensures compatibility with existing tooling and documentation.
When checking for security features or configurations in shell scripts, use precise pattern matching to avoid false positives and limit information exposure. Instead of broad substring matches that could match unintended content, use specific patterns and restrict output to only necessary information.
For example, when checking for Docker’s user namespace security feature:
# Avoid: broad matching that could have false positives
if [ ! -z "$(docker info 2>/dev/null | grep userns)" ]; then
# Better: precise matching with limited output
if docker info --format '{{json .SecurityOptions}}' 2>/dev/null | grep -q 'name=userns'; then
This approach reduces the risk of incorrectly identifying security features and minimizes information leakage by querying only the specific data needed for the security check.
Apply the principle of least privilege when managing credentials in CI/CD pipelines and application code. Use credentials that are limited to only the specific permissions required for the task, and remove credentials entirely when they’re not needed. This reduces security risk by limiting potential access if credentials are compromised and makes credential rotation easier.
For example, instead of using broad credentials that provide access to multiple services:
// Avoid: Using broad credentials with unnecessary permissions
withCredentials([
usernamePassword(credentialsId: 'orcaeng-hub.docker.com',
usernameVariable: 'REGISTRY_USERNAME',
passwordVariable: 'REGISTRY_PASSWORD')
])
Consider creating task-specific credentials or removing them entirely when accessing public resources:
// Better: Remove credentials when accessing public resources
// No credentials needed since fossa-analyzer is public on hub
Always evaluate whether credentials are actually necessary and ensure they grant only the minimum permissions required for the specific operation.
Maintain consistent formatting and style choices throughout your codebase to improve readability and maintainability. This includes consistent variable syntax, proper quoting practices, and organized list formatting.
Key practices:
${VAR} or don’t, but be consistent within the same file/project)Example of good formatting:
# Consistent variable quoting
docker build -f "${DOCKERFILE}" -t "${TAG}" --target "${DOCKER_BUILD_TARGET}" .
# Well-formatted package list
RUN pip install \
tox==2.1.1 \
virtualenv==16.2.0
Consistent formatting reduces cognitive load for code reviewers and makes diffs cleaner when modifications are needed.
Order Dockerfile instructions from least frequently changing to most frequently changing to maximize Docker layer caching efficiency and reduce build times in CI/CD pipelines.
Place stable elements like base images, system packages, and dependency installations early in the Dockerfile. Move frequently changing elements like source code copies, environment variables used only at runtime, and build artifacts toward the end.
Example of proper ordering:
# Stable base and dependencies first
FROM python:3.7.3-alpine3.9 AS build
RUN apk add --no-cache gcc git make
# Less frequently changing files
COPY requirements.txt .
COPY setup.py .
RUN pip install -r requirements.txt
# More frequently changing source code
COPY . .
# Runtime-specific variables last
ARG GIT_COMMIT=unknown
ENV DOCKER_COMPOSE_GITSHA=$GIT_COMMIT
RUN script/build/linux-entrypoint
This approach prevents cache invalidation of expensive operations like package installations when only source code changes, significantly improving CI/CD build performance.
Design data structures that accurately reflect the constraints and behavior of your problem domain. Choose data types that match the natural constraints of your values, and design structural patterns that align with expected usage.
For numeric fields, use unsigned types when values cannot be negative:
type FileTree struct {
Root *FileNode
Size int
FileSize uint64 // Use uint64 since file sizes cannot be negative
}
For tree structures, consider whether structural nodes (like root nodes) should participate in traversal or serve purely as organizational anchors. Document these design decisions to prevent confusion:
// never process the root node - it serves as a structural anchor
// keeping a single root node simplifies traversal compared to multiple roots
if currentNode == tree.Root {
continue // skip processing, but traverse children
}
This approach prevents bugs by encoding domain knowledge directly into the data structure design, making invalid states harder to represent and reducing the need for runtime validation.
Tests should not depend on external resources like real URLs, remote servers, or external APIs as these dependencies make tests flaky and unreliable. Instead, use local mock servers, stub responses, or dependency injection to simulate external behavior.
When tests require external resources, create local alternatives that provide the same interface. For example, instead of making HTTP requests to real URLs, spin up a local static file server or use mocked HTTP responses.
Example of problematic approach:
def test_down_url(self):
url = 'https://raw.githubusercontent.com/docker/compose/...'
# This creates a flaky test dependent on network and external service
Better approach:
def test_down_url(self, mock_server):
# Use a local mock server that serves the same content
url = f'http://localhost:{mock_server.port}/docker-compose.yml'
This principle also applies to mocking internal dependencies - when code paths change and new components are exercised, ensure proper mocks are in place rather than relying on early exits or bypassing the actual execution flow.
When code elements become complex—whether functions performing multiple operations or data structures with multiple attributes—they require proper documentation to aid understanding and maintenance. Complex functions should include docstrings explaining their purpose, parameters, and behavior. Data structures should document their attributes and types for clarity.
For example, a function like load() that processes configurations, validates schemas, builds services, and merges data should have a comprehensive docstring:
def load(config_details):
"""
Load and process configuration files into service dictionaries.
Args:
config_details: Tuple containing working directory and config objects
Returns:
List of processed service dictionaries ready for deployment
This function handles configuration preprocessing, validation,
service building, and config merging for multi-file setups.
"""
Similarly, classes like ConfigDetails and ConfigFile should document their attributes and types to ensure consistent usage across the codebase. This practice becomes especially important when the team lacks consistent documentation standards—establishing clear documentation for complex elements helps maintain code quality and developer productivity.