-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce cozy-proxy #615
Introduce cozy-proxy #615
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughA new release entry for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant GitHub
participant FileSystem
User->>Makefile: Trigger "update" target
Makefile->>FileSystem: Remove existing 'charts' directory
Makefile->>GitHub: Fetch latest cozy-proxy tag
GitHub-->>Makefile: Return latest version tag
Makefile->>GitHub: Download tarball for version tag
GitHub-->>Makefile: Provide tarball
Makefile->>FileSystem: Extract 'charts' directory
Makefile-->>User: Updated charts available
sequenceDiagram
participant HelmUser as "Helm User"
participant Helm
participant Chart
participant Kubernetes
HelmUser->>Helm: Execute helm install for cozy-proxy
Helm->>Chart: Read Chart.yaml and values
Helm->>Templates: Render manifests via helper templates
Templates-->>Helm: Generated Kubernetes manifests
Helm->>Kubernetes: Apply manifests (DaemonSet, RBAC, etc.)
Kubernetes-->>Helm: Resources deployed successfully
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/system/cozy-proxy/charts/cozy-proxy/templates/_helpers.tpl (1)
18-24
: LGTM! Consider adding recommended selector labels.The implementation follows Helm chart best practices. Consider adding recommended selector labels for better Kubernetes resource management.
Add these selector labels to improve resource management:
{{- define "cozy-proxy.labels" -}} helm.sh/chart: {{ include "cozy-proxy.name" . }}-{{ .Chart.Version | replace "+" "_" }} app.kubernetes.io/name: {{ include "cozy-proxy.name" . }} app.kubernetes.io/instance: {{ .Release.Name }} app.kubernetes.io/version: {{ .Chart.AppVersion }} app.kubernetes.io/managed-by: {{ .Release.Service }} +app.kubernetes.io/component: proxy +app.kubernetes.io/part-of: cozystack {{- end -}}packages/system/cozy-proxy/charts/cozy-proxy/values.yaml (1)
8-9
: Add examples for podAnnotations and podLabels.Consider adding commented examples for podAnnotations and podLabels to guide users.
- podAnnotations: {} - podLabels: {} + # -- Additional annotations for pods + podAnnotations: + # example.com/annotation: value + # -- Additional labels for pods + podLabels: + # example.com/label: valuepackages/system/cozy-proxy/Makefile (1)
7-11
: Refine the Update Target for Robustness and Clarity
Theupdate
target is implemented to remove the existingcharts
directory, fetch the latest tag, and extract the charts from the tarball. Consider adding an error check to verify a tag was found before proceeding. A brief comment on the awk expression (used to extract the version number) would also improve maintainability. For example:-update: - rm -rf charts - tag=$$(git ls-remote --tags --sort="v:refname" https://github.com/aenix-io/cozy-proxy | awk -F'[/^]' 'END{print $$3}') && \ - curl -sSL https://github.com/aenix-io/cozy-proxy/archive/refs/tags/$${tag}.tar.gz | \ - tar xzvf - --strip 1 cozy-proxy-$${tag#*v}/charts +update: + # Remove the existing charts directory. + rm -rf charts + # Retrieve the latest tag from the cozy-proxy repository. + tag=$$(git ls-remote --tags --sort="v:refname" https://github.com/aenix-io/cozy-proxy | awk -F'[/^]' 'END{print $$3}') + # Abort if no tag was found. + if [ -z "$$tag" ]; then echo "No tag found. Exiting update."; exit 1; fi + # Download and extract the charts from the tarball. + curl -sSL https://github.com/aenix-io/cozy-proxy/archive/refs/tags/$${tag}.tar.gz | \ + tar xzvf - --strip 1 cozy-proxy-$${tag#*v}/chartspackages/core/platform/bundles/paas-full.yaml (1)
53-58
: Approve and Enhance Dependency Array Formatting
The new release entry forcozy-proxy
in the PaaS configuration correctly specifies the dependencies on bothcilium
andkubeovn
. For improved readability and to satisfy YAML linters, consider adding a space after the comma in the array. For example:- dependsOn: [cilium,kubeovn] + dependsOn: [cilium, kubeovn]🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 57-57: too few spaces after comma
(commas)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/core/platform/bundles/distro-full.yaml
(1 hunks)packages/core/platform/bundles/paas-full.yaml
(1 hunks)packages/system/cozy-proxy/Chart.yaml
(1 hunks)packages/system/cozy-proxy/Makefile
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/Chart.yaml
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/templates/_helpers.tpl
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/templates/daemonset.yaml
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/templates/role.yaml
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/templates/rolebinding.yaml
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/templates/serviceaccount.yaml
(1 hunks)packages/system/cozy-proxy/charts/cozy-proxy/values.yaml
(1 hunks)packages/system/cozy-proxy/values.yaml
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/system/cozy-proxy/values.yaml
- packages/system/cozy-proxy/Chart.yaml
- packages/system/cozy-proxy/charts/cozy-proxy/Chart.yaml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/system/cozy-proxy/charts/cozy-proxy/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozy-proxy/charts/cozy-proxy/templates/rolebinding.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozy-proxy/charts/cozy-proxy/templates/role.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/core/platform/bundles/paas-full.yaml
[warning] 57-57: too few spaces after comma
(commas)
packages/system/cozy-proxy/charts/cozy-proxy/templates/daemonset.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (8)
packages/system/cozy-proxy/charts/cozy-proxy/templates/serviceaccount.yaml (1)
1-8
: LGTM! ServiceAccount implementation follows best practices.The ServiceAccount is correctly configured with proper conditional creation and labeling.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozy-proxy/charts/cozy-proxy/values.yaml (2)
6-9
: Review security implications of hostNetwork: true.Using hostNetwork: true gives pods direct access to the host's network namespace, which could have security implications.
Please ensure this is required for the proxy's functionality and document the security considerations.
1-4
: Consider using a more flexible image tag strategy.Using a fixed tag (v0.1.0) could lead to outdated images. Consider using a more flexible strategy or documenting the version policy.
Please verify the latest available version:
✅ Verification successful
🌐 Web query:
What is the latest version of cozy-proxy on GitHub (aenix-io/cozystack)?
Length of output: 2548
Update the image tag strategy to reflect the latest available version.
The image tag in the chart is currently set to a fixed value of v0.1.0, which is outdated since the latest release on GitHub (aenix-io/cozystack) is v0.24.0. Consider updating the default value or using a more flexible/dynamic approach to avoid future discrepancies.
- File to update:
packages/system/cozy-proxy/charts/cozy-proxy/values.yaml
- Recommendation: Either update to the latest version (v0.24.0) or employ a strategy that allows for easier updates (e.g., referencing a tag maintained in a CI/CD process or documented version policy).
packages/system/cozy-proxy/charts/cozy-proxy/templates/role.yaml (1)
1-12
: Consider using Role instead of ClusterRole for better security.The current implementation uses cluster-wide permissions. Consider:
- Document why cluster-wide access to services and endpoints is required
- If possible, use namespaced Role instead of ClusterRole for better security
Please clarify if cluster-wide access is necessary for the proxy's functionality.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozy-proxy/Makefile (1)
1-2
: Define Project Variables Clearly
Declares the project name and namespace appropriately for use in the Makefile.packages/system/cozy-proxy/charts/cozy-proxy/templates/rolebinding.yaml (1)
1-16
: Handle YAML Templating Syntax in Linters
The inclusion of Helm templating directives (such as{{- if .Values.rbac.create }}
) may trigger false positives from YAML linters. Ensure that your linter configuration is set to ignore or properly handle Helm template syntax so that these warnings do not obscure genuine issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozy-proxy/charts/cozy-proxy/templates/daemonset.yaml (1)
1-28
: Validate YAML Template with Linter Settings
The DaemonSet definition looks well structured. Similar to the rolebinding template, templating directives (for example, on line 6 where labels are included) might raise false positives in YAML linting. Please confirm that your CI configuration ignores these templating constructs. Also, the overall configuration (security context, image specifications, etc.) meets the deployment needs.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
packages/core/platform/bundles/distro-full.yaml (1)
34-40
: Verify New Release Entry forcozy-proxy
The release entry forcozy-proxy
is added with a release name ofcozystack
and chartcozy-cozy-proxy
, along with the dependency oncilium
and theoptional: true
flag. This appears consistent with the intended deployment configuration.
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit