git: add push.refspec to push using a refspec #514
Conversation
aryan9600
force-pushed
the
push-refspec
branch
3 times, most recently
from
98d44bd
to
2cbf9fd
Compare
|
@aristapimenta, could you please try these changes and see if it works as expected for you with Gerrit? The image can be pulled by running |
|
Sure! Is this time sensitive? I just left on PTO for the next two weeks, do we have time to do this when I'm back? |
hiddeco
added
enhancement
|
@aristapimenta no pressure take your time, but it'd be great to have your confirmation soon. enjoy your time off :) |
|
@aryan9600 I tested the change today, and with I'm gonna investigate further tomorrow to make sure this is not a configuration mistake |
|
This is the object: apiVersion: image.toolkit.fluxcd.io/v1beta1
kind: ImageUpdateAutomation
metadata:
name: podinfo
spec:
interval: 1m0s
sourceRef:
kind: GitRepository
name: flux-system
namespace: flux-system
git:
checkout:
ref:
branch: main
commit:
author:
email: fluxcdbot@users.noreply.github.com
name: fluxcdbot
messageTemplate: '{{range .Updated.Images}}{{println .}}{{end}}'
push:
refspec: HEAD:refs/for/main
update:
path: ./apps/podinfo/podinfo.yaml |
|
From the stack trace in the logs I posted above, looks like the error is specifically from the push: if err := gitClient.Push(pushCtx, pushConfig); err != nil {
return failWithError(err)
} |
|
The update path must be a directory, instead of a file path https://fluxcd.io/flux/components/image/imageupdateautomations/#update-strategy |
aryan9600
force-pushed
the
push-refspec
branch
2 times, most recently
from
2be3cfc
to
f0e3a36
Compare
|
@aristapimenta what do the ImageUpdate object events show? |
@stefanprodan did this change recently? I copied this config from a previous attempt which worked well with GitHub (but not Gerrit). Anyways, I updated to
This: |
|
I'm suspecting that this is because Gerrit has a |
|
i'd like to see the error that originates because of: if err := gitClient.Push(pushCtx, pushConfig); err != nil {
return failWithError(err)
}i don't know why the actual error is missing from the logs, but it should be in the events. Could you change the ImagePolicy in such a way that it'll be forced to push a commit? |
I think {
"level": "Level(-2)",
"ts": "2023-05-29T17:16:28.594Z",
"msg": "applying setter",
"controller": "imageupdateautomation",
"controllerGroup": "image.toolkit.fluxcd.io",
"controllerKind": "ImageUpdateAutomation",
"ImageUpdateAutomation": {
"name": "podinfo",
"namespace": "podinfo"
},
"namespace": "podinfo",
"name": "podinfo",
"reconcileID": "bd721ab7-a3c3-4400-a103-df248f5ff7e8",
"setter": "podinfo:podinfo",
"old": "ghcr.io/stefanprodan/podinfo:5.0.0",
"new": "ghcr.io/stefanprodan/podinfo:5.0.3"
} |
|
{
"level": "error",
"ts": "2023-05-29T17:16:30.977Z",
"msg": "already up-to-date",
"name": "podinfo",
"namespace": "podinfo",
"reconciler kind": "ImageUpdateAutomation",
"annotations": null,
"error": "error",
"stacktrace": "
github.com/fluxcd/pkg/runtime/events.(*Recorder).AnnotatedEventf
github.com/fluxcd/pkg/runtime@v0.35.0/events/recorder.go:137
github.com/fluxcd/pkg/runtime/events.(*Recorder).Eventf
github.com/fluxcd/pkg/runtime@v0.35.0/events/recorder.go:114
github.com/fluxcd/image-automation-controller/internal/controllers.(*ImageUpdateAutomationReconciler).event
github.com/fluxcd/image-automation-controller/internal/controllers/imageupdateautomation_controller.go:654
github.com/fluxcd/image-automation-controller/internal/controllers.(*ImageUpdateAutomationReconciler).Reconcile.func2
github.com/fluxcd/image-automation-controller/internal/controllers/imageupdateautomation_controller.go:165
github.com/fluxcd/image-automation-controller/internal/controllers.(*ImageUpdateAutomationReconciler).Reconcile
github.com/fluxcd/image-automation-controller/internal/controllers/imageupdateautomation_controller.go:412
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile
sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:122
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:323
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:274
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:235"
} |
|
since this PR no longer targets support for gerrit (ref: #509 (comment)), its unblocked and is ready for review |
|
@aryan9600 I'm testing @brovoca's message template here but I'm getting the same error. I'm gonna try to debug-print the generated message and see if look correct. |
|
@aryan9600 In fact, by using aryan9600#2 I was able to confirm that the commit message indeed contains a Below is a full sample of running with |
|
The error is returned here: https://github.com/go-git/go-git/blob/v5.7.0/remote.go#L174 |
|
I found the problem investigating https://github.com/go-git/go-git/blob/v5.7.0/remote.go#L720-L722 This in turn means that no https://github.com/go-git/go-git/blob/v5.7.0/remote.go#L174 Which is the error This is an easy problem to solve, just use The Gerrit server accepted the push and opened a review! ✔️ |
|
However looks like
|
|
it works, if both
while this solves the problem of multiple Patchsets for a single Change, it doesn't address of the issue of potential multiple Changes. since the |
|
@aryan9600 wrote:
Gerrit doesn't have a concept of "pull requests" as the tool uses "reviews" instead. See the Basic Gerrit Walkthrough — For GitHub Users which states:
Additionally, even if we'd push it to a separate branch, I don't see how that would prevent multiple patchsets from being created on that branch, because the main branch which the reconciler would clone still wouldn't contain the changes. Adding to that, now we'd have to merge that branch with main. Please help me if I'm missing some crucial detail here. @aryan9600 wrote:
I am the author of the message template that applies this method of calculating the Change-Id. First of all I want to point out the necessity of It is important that the IA controller doesn't generate the same Change-Id twice as that would try to send a new "patch set" to an already merged "review", which is why multiple "reviews" would be created in case image grafana/grafana in deployment.yaml first got bumped to 9.5.2 and the next time to 9.5.7, even if the 9.5.2 review hasn't been merged yet. Hope this helps bring clarity to the ways of working with Gerrit. |
the main branch won't but the branch specified in |
sorry, i meant a "Change" here. still getting used to Gerrit :) |
@aryan9600 this feels like the remote branch created by that said, I don't think this should block this PR. we can address this in a future change, as it requires the code to be able to have Gerrit-specific logic, which probably needs some preparation. we could probably just document this solution in the blog post along with the |
| @@ -427,6 +439,9 @@ Note that without force push in push branches, if the target branch is stale, th | |||
| be able to conclude the operation and will consistently fail until the branch is either deleted or | |||
| refreshed. | |||
|
|
|||
| If both `push.refspec` and `push.branch` are specified, then the reconciler will perform two push | |||
There was a problem hiding this comment.
Besides what it enables for Gerrit, I think this behavior is surprising and could potentially result in issues when people define a branch and refspec which are essentially equal to each other.
There was a problem hiding this comment.
maybe we should document that this will probably lead to an already up-to-date error?
There was a problem hiding this comment.
that this will probably
what is "this" in this case?
There was a problem hiding this comment.
the fact that if push.refspec and push.branch are essentially equal to each other (for eg: refs/heads/main:refs/heads/main and main), then the controller will fail while trying to perform the second push operation, since main will already be up to date with all the changes
There was a problem hiding this comment.
I thought the reconciliation would return earlier in that case, since the commit would be empty or something, rather than trying to push
There was a problem hiding this comment.
why would the commit be empty? both push operations are happening in a single reconciliation.
aryan9600
force-pushed
the
push-refspec
branch
from
051844c
to
a6551e3
Compare
aryan9600
force-pushed
the
push-refspec
branch
2 times, most recently
from
5b935e3
to
e216fa2
Compare
Add `.spec.git.push.refspec` to allow specifying a refspec to be used for performing a push operation. If specified alongside `.spec.git.push.branch`, two push operations, one for each specified push configuration will be performed. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
aryan9600
force-pushed
the
push-refspec
branch
from
e29ae1d
to
f7c5f69
Compare
Add
.spec.git.push.refspecto allow specifying a refspec to be used for performing a push operation. If specified alongside.spec.git.push.branch, two push operations, one for each specified push configuration will be performed.TODO: add docs for how to use this with Gerrit, will do that once we confirm that it works as expected.Fixes: #509
Depends on: fluxcd/pkg#550