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
#164 Send events on image change #167
Conversation
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
==========================================
+ Coverage 68.20% 68.85% +0.65%
==========================================
Files 19 19
Lines 1343 1384 +41
==========================================
+ Hits 916 953 +37
- Misses 344 346 +2
- Partials 83 85 +2
Continue to review full report at Codecov.
|
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.
Hey, awesome @magmax! Pretty great for your first lines of Go, actually! I only have a few comments (see inline) and also the following:
- We need to give the ServiceAccount for the Image Updater appropriate Kubernetes RBAC permissions to create Events. The
Role
is part of the manifests and can be found here. Can you please provide proper authorization there? - I feel that emitting events should be enabled by default, but optionally can be disabled. It would be great to have a command line switch to disable it (e.g.
--disable-kube-events
). The main command can be found here and the flags are defined here. We provide default values to most flags that are override-able using environment variables (e.g. usingenv.GetBoolVal()
), and then map those to keys in theargocd-image-updater-config
ConfigMap, as shown here. I think this one should be settable byIMAGE_UPDATER_KUBE_EVENTS
with a ConfigMap key ofkube.events
.
I did the change. I didn't tested it under real fire because I have no repository accessible from this computer and it would take me a while to do it. |
pkg/argocd/update.go
Outdated
annotations[fmt.Sprintf("%s/oldtag", c.imageName)] = c.oldTag.TagName | ||
annotations[fmt.Sprintf("%s/newtag", c.imageName)] = c.newTag.TagName |
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.
On a second thought, I'm not sure whether this will work reliably. The name parts of annotations are rather strict, and must follow a rule:
regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')
Also, the name must be max 63 characters long. I think we must either make sure that the image name is properly normalized, look for another name for the annotation (and put the image name in its value) or think about a completely another solution (e.g. putting the details into the event's Message
).
Is there a specific reason that you use an annotation for storing the updated images?
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.
Not really. I want just to "pass parameters" between the argocd-image-updater and a listener through events.
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.
New approach. Now annotations have less randomness and a fixed size around 40 characters
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.
Awesome! I tested it successfully, the event now looks like:
Name: guestbook-multi.166fdad08646fd49
Namespace: argocd
Labels: <none>
Annotations: argocd-image-updater.image-0/image-name: heptio-images/ks-guestbook-demo
argocd-image-updater.image-0/new-tag: 0.2
argocd-image-updater.image-0/old-tag: 0.1
argocd-image-updater.image-1/image-name: coredns/coredns
argocd-image-updater.image-1/new-tag: 1.8.3
argocd-image-updater.image-1/old-tag: 1.8.0
API Version: v1
Count: 1
...
Involved Object:
API Version: argoproj.io/v1alpha1
Kind: Application
Name: guestbook-multi
Namespace: argocd
...
Message: Successfully updated application 'guestbook-multi'
...
Source:
Component: ArgocdImageUpdater
Type: Normal
I'm not sure whether it's intended to use the image name without the registry prefix (the Image Updater's API is possibly not good documented in that regard), but I'd suggest to change
changeList = append(changeList, change{updateableImage.ImageName, updateableImage.ImageTag, containerImageNew.ImageTag})
to
changeList = append(changeList, change{updateableImage.GetFullNameWithoutTag(), updateableImage.ImageTag, containerImageNew.ImageTag})
at the place where you store the image information to be propagated to the event.
And I also guess this would be the last change required before merge 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking in using annotations to compose a message on slack. So allowing the user to decide might be a really cool idea :)
So, I finally decided to add both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that solution as well 👍
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.
LGTM!
Thanks for this contribution @magmax - much appreciated!
Also, you said it's your first steps into Golang, but I have to say great job on the code and thanks for constructively working through the review with me!
Thank you very much for your comments and support. |
If
|
Sends an event on any image change (fixes #164)
I thought in re-using some argo-cd code, but finally I decided to copy and adapt it. It is the code related to send the event.
The namespace used is the one where argocd-image-updater is running in order to reduce the permissions required (a Role instead a ClusterRole).
I had to add some argo-cd dependencies to the kubernetes package, what is... ugly. Maybe you know a better way to do it.
Finally, understand this is my first golang contribution... I'm open to changes XD
NOTE: I didn't tested it under real fire. I have no docker repository accessible from my laptop.