fix: return revision from getGitGeneratorInfo

[reiniertimmer]

It seems that the webhook endpoint was not working completely as expected when tracking a repository branch (other than the default branch).

It turns out that the revision that is being used to compare against the targetRevision is always empty. So the test on https://github.com/argoproj/applicationset/blob/master/pkg/utils/webhook.go#L246 would always return false.

The revision field is already being evaluated on https://github.com/argoproj/applicationset/blob/master/pkg/utils/webhook.go#L146, but for some reason it was never included in the response.

After this change, we verified this on our own installation and now the webhook processing does recognize the correct branches on push events from GitHub.

[reiniertimmer]

I see that this puts back the errors package that was addressed in #509

Unfortunately, the build did not work for me without this dependency (which was re-introduced by running go mod tidy as suggested by the output of the build command).

I'm not really sure what is causing this. @crenshaw-dev would you know anything about this?

[crenshaw-dev]

Strange. What was the error message when building?

[reiniertimmer]

Strange. What was the error message when building?

go: updates to go.mod needed; to update it:
	go mod tidy

See workflow run https://github.com/argoproj/applicationset/runs/5333719894?check_suite_focus=true

Once I ran the go mod tidy command and committed the CI process did pass. It wasn't completely clear to me at first that this was the cause of the error, but it did resolve the issue somehow.

[crenshaw-dev]

Yep, that's weird. Opened a PR to fix just that. #525

[reiniertimmer]

I'm not too familiar with the go ecosystem, but could it be because of https://github.com/argoproj/applicationset/blob/master/pkg/services/scm_provider/github.go#L5 that go mod tidy wants to keep this line?

It seems that the errors package is still being used, therefore tidying the modules will put this back.

However, I tested removing this import and code, and still got the same error, strangely enough

[crenshaw-dev]

Yeah, I guess it's a transient dependency. I'm not sure when/why go mod decides it needs indirect dependencies explicitly listed in go.mod, but guess we gotta trust it.

[crenshaw-dev]

LGTM! Would be interested to see a test added, if that's not too tricky. But for a trivial change, I think this is fine. :-)

[reiniertimmer]

I'll have a look. It seems that webhook_test.go contains some test cases, I could probably add a case for testing push to the non-default branch. In the old situation that would result in a failure to update, in the new case it should pass.

[reiniertimmer]

@crenshaw-dev it took a while before I got some time, but I added a test case. It wasn't that tricky, but I needed to add the revision in the mock git-generator (default it to master), because it was not used anywhere in the tests. I'm guessing the test suite was a bit limited in that sense.

Anyway, when removing my fix (the one line) the test fails, but with the change it succeeds as expected.

Who should I include to get this merged? @wtam2018 would you mind having a look?

[reiniertimmer]

@jgwest would you mind having a look?

[rishabh625]

LGTM to me, moving this in argocd

[rishabh625]

@reiniertimmer : Can u please move this PR into argocd as applicationset is moved. If u find it laborious, i can do it on your behalf with your consent

[reiniertimmer]

@rishabh625 thanks! Sure can do, I will have a look to find some time to move it over