Skip to content
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

Update ArgoCD Rollouts to 1.0.1 to fix a compatibility issue #236

Merged

Conversation

tete17
Copy link
Contributor

@tete17 tete17 commented May 31, 2021

This changed also required to update the k8s machinery.

All the newly fixed packages are required because ArgoCD Rollouts still depend on k8s.io/kubernetes

On future release they will drop the dependency and we can also remove all the replace block.

Should fix #232

@tete17 tete17 force-pushed the Update-dependencies-for-Argo-Rollouts branch from 18420e6 to 71fdb53 Compare May 31, 2021 13:36
@stakater-user
Copy link
Contributor

@tete17 Image is available for testing. docker pull stakater/Reloader:SNAPSHOT-PR-236-18420e62

@stakater-user
Copy link
Contributor

@tete17 Image is available for testing. docker pull stakater/Reloader:SNAPSHOT-PR-236-71fdb53c

@jessesuen
Copy link

I just made a similar comment here: #232 (comment), but it may be wise to switch to using the kubernetes dynamic client to get/list rollouts rather than importing the generated client in the Argo Rollouts repo, which causes some obnoxious golang dependencies. Argo Rollouts has some kubernetes imports which are hard to get rid of.

The dynamic client approach will also automatically give support for v1.0 Rollouts (this PR is only adding support for v0.10)

@tete17
Copy link
Contributor Author

tete17 commented Jun 4, 2021

Hi @jessesuen I am really happy you like the support of Rollouts here. I implemented for my company since we are heavy users of both at the same time and they are both awesome.

I had to update the dependency version because we faced a bug in our system that prevented to updated the Rollout if the wait field was a string, due to an incompatibility between Rollout versions. Indeed I have been following argoproj/argo-rollouts#473 to avoid having all those nasty replace clauses but it seems it will take longer to fix.

I am more than happy to try out the dynamic client approach (it may be even beneficial to the project since they also depend on openshift project and they don't provide releases 😠 )

I am afraid I am also a half noobie to k8s api machinery and golang. I have been mostly following this guide https://www.martin-helmich.de/en/blog/kubernetes-crd-client.html and using your provided client to query the Rollouts, can you maybe provide me some docs or any info on how to get a dynamic client working please?

@tete17
Copy link
Contributor Author

tete17 commented Jun 4, 2021

PS: Since you like seeing your project supported guess what tool are we using for rendering our yaml? https://github.com/GoogleContainerTools/skaffold/blob/f280aa8325283a0f080989acef120c5597ebc4b7/pkg/skaffold/kubernetes/manifest/visitor.go#L42

@tete17 tete17 force-pushed the Update-dependencies-for-Argo-Rollouts branch from 34e0b5e to 7643a27 Compare June 4, 2021 16:32
@stakater-user
Copy link
Contributor

@tete17 Image is available for testing. docker pull stakater/Reloader:SNAPSHOT-PR-236-34e0b5ed

@tete17
Copy link
Contributor Author

tete17 commented Jun 4, 2021

Quick update @jessesuen I gave it a try your idea to use the dynamic client.

I followed the following example https://github.com/kubernetes/client-go/blob/master/examples/dynamic-create-update-delete-deployment/main.go

I don't think it is impossible but It becomes much more cumbersome for now than adding a bunch of replace clauses. The reason being is the current implementation of Rollouts depends on modifying the current Deployment, DaemonSet, Rollout... in place. It basically uses the multiple functions each implementations provide to return references to []v1.Containers whose environment variables get modified in place. Unfortunately this doesn't play nicely with unstructured.Unstructured since I need to perform a copy of any values I want to return to the caller. The controller will modify that copy thinking it behaves as a reference and then return to the k8s api the unmodified object.

In order to get rid of the dependency a significant amount of refactoring needs to happen to change this behaviour.

For me particularly we need this support a bit sooner than that. We are already affected by a bug and this feels like a good and quick remediation plan for now.

@stakater-user
Copy link
Contributor

@tete17 Image is available for testing. docker pull stakater/Reloader:SNAPSHOT-PR-236-7643a27f

@tete17 tete17 changed the title Update ArgoCD Rollouts to 0.10.2 to fix a compatibility issue Update ArgoCD Rollouts to 1.0.1 to fix a compatibility issue Jun 4, 2021
@faizanahmad055 faizanahmad055 merged commit a8a68ae into stakater:master Jun 13, 2021
@tete17 tete17 deleted the Update-dependencies-for-Argo-Rollouts branch June 23, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Argo Rollouts canaryMetadata is gone once Reloader trigger Rollout change
4 participants