-
Notifications
You must be signed in to change notification settings - Fork 6
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: adds controller to allow addon to reconcile on changes #46
Conversation
Pull Request Test Coverage Report for Build 9368976680Details
💛 - Coveralls |
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
This reverts commit 0e9894e.
It seems I have permission to push to this branch. I added a commit and reverted it. Added my changes in JoaoBraveCoding#11 |
* Move the watcher cli application to be a go routine Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Update internal/controllers/watcher/controller.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Update internal/controllers/watcher/controller.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Update main.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Update main.go Co-authored-by: Joao Marcal <joao.marcal12@gmail.com> * Add suggestion Signed-off-by: Israel Blancas <iblancasa@gmail.com> --------- Signed-off-by: Israel Blancas <iblancasa@gmail.com> Co-authored-by: Joao Marcal <joao.marcal12@gmail.com>
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
What is missing to move from |
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.
@iblancasa some points to improve
FYI: Tested the current version of this PR with some of my suggestions together with #48 and everything worked out 👌 |
* If a secret is updated, check if there is any reference to it in any of the ManifestWorks * Change some comments Signed-off-by: Israel Blancas <iblancasa@gmail.com>
@JoaoBraveCoding I added the changes yesterday. I tested the changes and I think everything is working fine. Is ok if we move to |
@iblancasa yes please move to ready for review. I am constantly skim over this and would like to add my review. Thanks for all the dedication and work you both put in here |
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
deploy/kustomization.yaml
Outdated
@@ -1,6 +1,9 @@ | |||
images: | |||
- name: controller | |||
newName: quay.io/rhobs/multicluster-observability-addon | |||
newName: quay.io/jmarcal/multicluster-observability-addon |
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.
Remember to revert this
@JoaoBraveCoding LGTM. Is there anything else to be implemented? |
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 besides the comments from @JoaoBraveCoding related to #48
Just tested once again and everything works! Merging as per the approved from @periklis and @iblancasa. |
This PR introduces a controller that will watch for resources used for the addon configuration and it will annotate the corresponding ManifestWorks triggering a reconciliation