chore: change the comment for defaultNamespace in values.yaml

[salonig23]

Ticket

Description

stop supporting resource_manager.namespace and use release namespace as default instead of "default" in helm deployements.

Test Plan

no testing required

Checklist

• Changes have been manually QA'd
• New features have been approved by the corresponding PM
• User-facing API changes have the "User-facing API Change" label
• Release notes have been added as a separate file under docs/release-notes/
  See Release Note for details.
• Licenses have been included for new code which was copied and/or modified from any external code

[netlify]

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	e14a0d0
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66b27e8e72c62e0008bdfda0

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.05%. Comparing base (786f258) to head (e14a0d0).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9793      +/-   ##
==========================================
- Coverage   54.07%   54.05%   -0.03%     
==========================================
  Files        1260     1260              
  Lines      155574   155565       -9     
  Branches     3503     3504       +1     
==========================================
- Hits        84120    84083      -37     
- Misses      71308    71336      +28     
  Partials      146      146              

Flag	Coverage Δ
backend	44.90% <ø> (-0.07%)	⬇️
harness	72.62% <ø> (ø)
web	53.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/internal/config/resource_manager_config.go	67.21% <ø> (-0.23%)	⬇️
...nal/rm/kubernetesrm/kubernetes_resource_manager.go	27.89% <ø> (+0.16%)	⬆️

... and 6 files with indirect coverage changes

[amandavialva01]

Minor change to the comment requested, but since this PR is currently a 1 word change for the defaultNamespace comment, I took a look at how defaultNamespace gets set in master-config.yaml, and we should probably add a coalesce as follows.

{{- if $defaultNamespace := coalesce .Values.resourceManager.defaultNamespace .Release.namespace }}
     default_namespace: {{ quote $defaultNamespace }}
 {{- end }}

This way, the default namespace that users previously had set for their k8s clusters gets preserved. Can we make that change here since it's on the topic of the same field whose comment is being modified?

[amandavialva01]

Minor change to the comment requested, but since this PR is currently a 1 word change for the defaultNamespace comment, I took a look at how defaultNamespace gets set in master-config.yaml, and we should probably add a coalesce as follows.

{{- if $defaultNamespace := coalesce .Values.resourceManager.defaultNamespace .Release.namespace }}
     default_namespace: {{ quote $defaultNamespace }}
 {{- end }}

This way, the default namespace that users previously had set for their k8s clusters gets preserved. Can we make that change here since it's on the topic of the same field whose comment is being modified?

cc @hkumar92 do we want this change?
Currently (without this change) when users helm upgrade with our newest helm chart, workspace<>namespace mappings indirectly created (with wksp<>rp , rp<>namespace mappings) will be "lost" since we now ignore the rp<>ns mappings. So unless the user updates their values.yaml to specify a resourceManager.defaultNamespace upon helm upgrade, their workspaces would all be bound to the default namespace which is mentioned in our documents.
However, we may not want this "default" namespace binding since previously, the namespace field of the helm chart was set to the release namespace, .Release.Namespace. Hence the change suggested here.
WDYT? Should we leave things as is and where an upgrade binds workspaces to the default namespace, or do we want previous (indirectly configured) workspace-namespace bindings to be carried over provided the user doesn't directly specify a default_namespace in values.yaml?

[hkumar92]

Minor change to the comment requested, but since this PR is currently a 1 word change for the defaultNamespace comment, I took a look at how defaultNamespace gets set in master-config.yaml, and we should probably add a coalesce as follows.

{{- if $defaultNamespace := coalesce .Values.resourceManager.defaultNamespace .Release.namespace }}
     default_namespace: {{ quote $defaultNamespace }}
 {{- end }}

This way, the default namespace that users previously had set for their k8s clusters gets preserved. Can we make that change here since it's on the topic of the same field whose comment is being modified?

cc @hkumar92 do we want this change? Currently (without this change) when users helm upgrade with our newest helm chart, workspace<>namespace mappings indirectly created (with wksp<>rp , rp<>namespace mappings) will be "lost" since we now ignore the rp<>ns mappings. So unless the user updates their values.yaml to specify a resourceManager.defaultNamespace upon helm upgrade, their workspaces would all be bound to the default namespace which is mentioned in our documents. However, we may not want this "default" namespace binding since previously, the namespace field of the helm chart was set to the release namespace, .Release.Namespace. Hence the change suggested here. WDYT? Should we leave things as is and where an upgrade binds workspaces to the default namespace, or do we want previous (indirectly configured) workspace-namespace bindings to be carried over provided the user doesn't directly specify a default_namespace in values.yaml?

I think it makes sense for us to default to the releaseNamespace if the user doesn't define anything instead of binding to default

[tara-det-ai]

Suggested change

	otherwise bound to the release namespace. We fall back to the default Kubernetes namespace,
	otherwise bound to the release namespace. For non-helm Determined deployments, it falls back to the default Kubernetes namespace, ``default``.

[salonig23]

fixed!

[tara-det-ai]

suggested edit

[kkunapuli]

LGTM. I know this is "chore" but it's also potentially user facing; do you think we need a release note?

[salonig23]

LGTM. I know this is "chore" but it's also potentially user facing; do you think we need a release note?

I had added release notes directly to the release notes PR!