-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add annotations through values #181
Conversation
/code pr |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
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.
🤖
v1 charts/applicationset/Chart.yaml:
- Version has been updated to 0.14.0 (minor change and aligned with SemVer)
v2 charts/applicationset/README.md:
- Version has been updated to 0.14.0 (minor change and aligned with SemVer)
charts/applicationset/templates/github-pr.yml:
- Added global application annotations to all created applications. This allows to streamline defining common annotations.
charts/applicationset/templates/gitlab-mr.yml:
- Added global application annotations to all created applications. This prevents repetitive code.
charts/applicationset/values.yaml:
- Added a new entry under globals for annotations allowing centralized management of annotations across applications.
No major issues or suggestions in this PR, good use of comment and adding functionality through values.yaml. Code formatting is consistently and easy to read. This should greatly benefit applications that utilize these global defaults.
Please rebase branch to update local changes with the latest remote code.
> HuggingFaceH4/starchat2-15b-v0.1 [pr | 0]
/code pr |
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.
🤖
Here's my review of the changes:
Good Changes:
- Moving annotations to a configurable map in values.yaml provides better flexibility for customisation
- Proper version bump for feature addition (0.13.6 -> 0.14.0)
- Removal of hardcoded notification channel in favour of configurable annotations
Suggestions:
- In
values.yaml
, consider adding validation for the Slack channel name:
globals:
annotations:
# @schema-pattern: ^[a-zA-Z0-9_-]+$
notifications.argoproj.io/subscribe.on-deleted.slack: argocd
- In GitHub/GitLab template files, improve annotation indentation:
metadata:
annotations:
{{- with $globals.annotations }}
{{- toYaml . | nindent 8 }} # Currently over-indented
{{- end }}
Minor Issues:
README.md
:
- Duplicate ingress.enabled parameter in the example configuration
No other significant issues found in the modified code.
> claude-3-5-sonnet-20241022 [pr | 0]
f6fcf90
to
b31cb04
Compare
This allows us to add additional annotations to applications via globals
b31cb04
to
5e3d5ee
Compare
/code pr |
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.
🤖
Commit Comments
charts/applicationset/Chart.yaml
- Updated version to 0.14.0:
-version: &version "0.13.6" +version: &version "0.14.0"
charts/applicationset/README.md
- Updated version and app version:
-![Version: 0.13.3](https://img.shields.io/badge/Version-0.13.3-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 0.13.3](https://img.shields.io/badge/AppVersion-0.13.3-informational?style=flat-square) +![Version: 0.14.0](https://img.shields.io/badge/Version-0.14.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: 0.14.0](https://img.shields.io/badge/AppVersion-0.14.0-informational?style=flat-square)
charts/applicationset/templates/github-pr.yml
- Moved notification settings to globals.annotations:
-notifications.argoproj.io/subscribe.on-deleted.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-deployed.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-health-degraded.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-sync-failed.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-sync-running.slack: {{ $notificationChannel }} +{{- with $globals.annotations }} +{{- toYaml . | nindent 8 }} {{- end }}
charts/applicationset/templates/gitlab-mr.yml
- Moved notification settings to globals.annotations:
-notifications.argoproj.io/subscribe.on-deleted.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-deployed.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-health-degraded.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-sync-failed.slack: {{ $notificationChannel }} -notifications.argoproj.io/subscribe.on-sync-running.slack: {{ $notificationChannel }} +{{- with $globals.annotations }} +{{- toYaml . | nindent 8 }} {{- end }}
charts/applicationset/values.yaml
- Added annotations section in globals:
globals: annotations: notifications.argoproj.io/subscribe.on-deleted.slack: argocd notifications.argoproj.io/subscribe.on-deployed.slack: argocd notifications.argoproj.io/subscribe.on-health-degraded.slack: argocd notifications.argoproj.io/subscribe.on-sync-failed.slack: argocd notifications.argoproj.io/subscribe.on-sync-running.slack: argocd
Enhancements
-
Consistent Notification Channels:
- Ensure that all notification channels are consistent across different templates and values files. This can be achieved by defining them once in the
globals.annotations
section.
- Ensure that all notification channels are consistent across different templates and values files. This can be achieved by defining them once in the
-
Documentation Updates:
- Update the README.md to reflect the new version and any changes made to the chart.
-
Schema Validation:
- Add schema validation for the annotations in the values.yaml file to ensure that only valid keys are used.
Issues
-
File Consistency:
- The
globals.notificationChannel
field is removed from values.yaml but still referenced in README.md. Ensure that all references are updated or removed.
- The
-
Template Consistency:
- The notification settings in github-pr.yml and gitlab-mr.yml should be consistent with the new annotations defined in values.yaml.
Example
To add more annotations dynamically, you can extend the globals.annotations
section in values.yaml:
globals:
annotations:
notifications.argoproj.io/subscribe.on-deleted.slack: argocd
notifications.argoproj.io/subscribe.on-deployed.slack: argocd
notifications.argoproj.io/subscribe.on-health-degraded.slack: argocd
notifications.argoproj.io/subscribe.on-sync-failed.slack: argocd
notifications.argoproj.io/subscribe.on-sync-running.slack: argocd
# Add more annotations as needed
Then, in your templates (github-pr.yml and gitlab-mr.yml), use the globals.annotations
variable to inject these annotations:
{{- with $globals.annotations }}
{{- toYaml . | nindent 8 }}
{{- end }}
This approach ensures that all notification settings are managed centrally and consistently across different templates and environments.
> llama-3.1-sonar-small-128k-chat [pr | 2]
/code pr |
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.
🤖
Subject: [REVIEW] feat(adds annotations through values): Allows user to add extra annotations in applications
This patch introduces the ability to add extra annotations to applications through values. The additional annotations can be set globally using the globals.annotations in values.yaml file, and they will be applied to all new applications created.
The impacts are as follows:
- Enhancements: Users can set custom Slack notification subscriptions for specific action types using the provided annotations.
- Bug fixes: No bug fixes are included in this patch.
- Documentation changes: Readme has been updated to make it clear that extra annotations can now be added via values file.
I recommend that you review the following files:
- charts/applicationset/Chart.yaml
- charts/applicationset/README.md
- charts/applicationset/templates/github-pr.yml
- charts/applicationset/templates/gitlab-mr.yml
- charts/applicationset/values.yaml
Please let me know if you need any further clarifications or have any suggestions.
> HuggingFaceH4/starchat2-15b-v0.1 [pr | 1]
This allows us to add additional annotations to applications via globals
What this PR does / why we need it:
Types of changes
[ ] Breaking change (fix or feature that would cause existing functionality to change)Which issue/s this PR fixes
How Has This Been Tested?
Checklist: