Integration of mvdkleijn/atlantis and florianbeisel/atlantis efforts

[mvdkleijn]

@florianbeisel Here's my initial merge of your stuff into mine. Let's discuss from here.

Note: I use my client implementation for now since I believe it should be its own package and my client implements paging.

Status:

• GetModifiedFiles()
• CreateComment()
• ReactToComment()
• HidePrevCommandComments()
• PullIsApproved()
• PullIsMergeable()
• UpdateStatus()
• DiscardReviews()
• MergePull()
• MarkdownPullLink()
• GetTeamNamesForUser()
• GetFileContent()
• SupportsSingleFileDownload()
• GetCloneURL()
• GetPullLabels()
• Accepts events
• Uses and checks security header
• Config flags created
• Manual testing is OK
• Unit testing was executed
• Broken tests fixed
• New tests added
• Added gitea/client_test.go
• Final "we're both happy with this" check done

[mvdkleijn]

I realized my comment was a little terse since I had a little time crunch. My idea is that we can use this PR as kind of a discussion platform and once we agree on the approach we want to take, we can merge it into the PR branch.

A lot of what we did is very similar. I would like to have the client as a distinct package though since that just seems cleaner. I migrated over your HidePrevCommandComments() implementation and added some other adjustments to the client.

I also believe a lot of the code that's now spread around the Atlantis codebase probably belongs in the clients. For example, I moved the GetPullRequest() function into the client and made DefaultCommandRunner use the client. Though cleanup like that for other clients / the codebase itself should be a new PR in the future.

[florianbeisel]

My idea is that we can use this PR as kind of a discussion platform
That works well for me 👍

I was also thinking about moving the gitea client into its own package but opted not to do it initially, but wanted to make it a bit easier to copy and paste from other clients without thinking about referencing a new package. But its no doubt the better way to move all the stuff into a gitea package.

I think you are right, that we should try to make our changes as cleanly as possible (the GetPullRequest() function in DefaultCommandRunner is an excellent example for that) but we should for the moment focus on a clean implementation of our functionality and keep refactoring the other clients for the future.

[mvdkleijn]

My idea is that we can use this PR as kind of a discussion platform
That works well for me 👍

👍

I was also thinking about moving the gitea client into its own package but opted not to do it initially, but wanted to make it a bit easier to copy and paste from other clients without thinking about referencing a new package. But its no doubt the better way to move all the stuff into a gitea package.

👍

I think you are right, that we should try to make our changes as cleanly as possible (the GetPullRequest() function in DefaultCommandRunner is an excellent example for that) but we should for the moment focus on a clean implementation of our functionality and keep refactoring the other clients for the future.

I agree completely. ❤️ We'll focus on the Gitea client and try to do that as nicely as we can. We can always opt to do extra PRs in the future if and when we have time and energy to clean up the rest of the clients / codebase a little.

For now I'll see if I can get this current branch working locally and fix some things as I come across them.

I've added you as a collaborator to this repo, so you should have access too I believe. (I'll double check right after this comment)

Is there a particular thing you want to look at?

[mvdkleijn]

@florianbeisel I just ran this branch locally against my locally hosted Gitea (Forgejo actually but that's basically the same thing)...

I ran the following test manually which seemed to all work:

• Creating a PR (led to Atlantis plan comment)
• Added comment atlantis plan -d .
• Added comment atlantis unlock
• Added comment atlantis help
• Added comment atlantis apply -d .
• Added comment atlantis apply
• All of the above comments appeared to have worked properly and the plan was applied.
• Renamed the PR (led to replan)
• I also saw the googly eyes comment reaction from Atlantis
• automerge (worked after I fixed my atlantis server config)

What I did not see

• policies comments

I'll take a look at those to see if they should happen at all.

Things are looking good however.

[florianbeisel]

I know from the changes I made, that webhook signatures aren't implemented at all. I think I'll be working on that later tonight.

I began implementing this yesterday but ran into some strange issue with the command line flag being recognized (there is a hard check to abort on unknown flags) but not being made available in the userconfig. Funnily enough, I could add flag by another name just fine.

I think I'll attach a debugger tonight and just see what our combined branch does with our flags.
Funnily enough I was certain there was function to verify the signature in the sdk. But I can't seem to find it now.

[mvdkleijn]

I think I'll attach a debugger tonight and just see what our combined branch does with our flags. Funnily enough I was certain there was function to verify the signature in the sdk. But I can't seem to find it now.

You mean https://pkg.go.dev/code.gitea.io/sdk/gitea#VerifyWebhookSignature ?

[florianbeisel]

Yes, that's the one - good find. I somehow landed on the old pkg.go.dev page when the SDK primarily lived on Github.

One thing I noticed in your current branch:

if r.Header.Get(giteaHeader) != "" {
  if !e.supportsHost(models.Gitea) {
	e.respond(w, logging.Debug, http.StatusBadRequest, "Ignoring request since not configured to support Gitea")
	return
  }
e.Logger.Debug("handling Gitea post")
e.handleGithubPost(w, r)
return

Did you use VCSEventsController.handleGithubPost purposefully? I think this should work just fine because of the fact that the webhooks are basically compatible between each other. I'm very much for not reinventing the wheel here but I'm wondering whether we should implement handleGiteaPost so should there in the future be any drift between the two. What are your thoughts on that?

[florianbeisel]

Looking a bit more into this, I think this might lead to problems down the line.
From my understanding (though I've not tried it in any way) atlantis should be able to support multiple forges (ie Github, Gitea, Gitlab) at the same time. The way we have it implemented now would mean that Gitea and Github always need to share the same webhook secret when coexisting.

[mvdkleijn]

Did you use VCSEventsController.handleGithubPost purposefully? I think this should work just fine because of the fact that the webhooks are basically compatible between each other. I'm very much for not reinventing the wheel here but I'm wondering whether we should implement handleGiteaPost so should there in the future be any drift between the two. What are your thoughts on that?

Currently I'm working on the test-merge branch only. In there I no longer use handleGithubPost but your handleGiteaPost. That is fine by me. I originally was using handleGithubPost to just try and get the Gitea client working, but that wasn't productive.

So yeah, I agree we should use handleGiteaPost going forward.

[mvdkleijn]

Looking a bit more into this, I think this might lead to problems down the line. From my understanding (though I've not tried it in any way) atlantis should be able to support multiple forges (ie Github, Gitea, Gitlab) at the same time. The way we have it implemented now would mean that Gitea and Github always need to share the same webhook secret when coexisting.

As far as I'm concerned, we treat Github and Gitea as two separate, incompatible forges. 😄 Also see my comment above.

[florianbeisel]

What I did not see

policies comments

I tested the comments per se, but I currently have no valid policy configuration to test against. At least my invalid configuration produces a Policy Check Error and the Statuschecks prevent the PR from being merged.

[mvdkleijn]

What I did not see
policies comments

I tested the comments per se, but I currently have no valid policy configuration to test against. At least my invalid configuration produces a Policy Check Error and the Statuschecks prevent the PR from being merged.

Similar on my end. I say once we're mostly happy with this branch, I update the PR with this and then we ask people to help test it out. There will probably be some work coming out of PR reviews anyway I expect.

I added a TODO list in the description of this PR as a sort of reminder of what we need to do to get this into an acceptable state. Feel free to add to it.

I'll start off by checking the client interface functions and checking them off where possible.

[florianbeisel]

My focus today was on testing the functions that are "hidden" behind command flags and which are not triggered on a normal run.

Now known to be working:

HidePrevCommandComments() needs --hide-prev-plan-comments
DiscardReviews() needs --discard-approval-on-plan
PullIsApproved()
PullIsMergeable()

UpdateStatus() is responsible for publishing commit status decoration which is used in Gitea for the status checks and signify whether checks passed and (depending on branch protection configuration) whether the PR can be merged. Works.

GetCloneURL() as far as I can see is primarily used for use with the API since when invoking via a webhook we have the relevant context to know the CloneURL already. I have integrated my original function and enabled our client to be available for API requests via http://atlantis/api/plan etc. Not much to say, it works.

GetFileContent() is used in very specific situations where atlantis tries not to redownload the whole repository if nothing has changed I believe. Similar to Github Gitea returns the file content base64 encoded. I added the decoding before processing the file further. Works.

I have looked a bit into GetTeamNamesForUser() and decided that this is a low(er) priority for me currently since we would be the only client outside of the github client that implements that. I think implementing it should be possible but a bit tedious, since we would need to get the owner of the repository, verify its an organisation and than get the teams.

[florianbeisel]

GetPullLabels() is used with the --disable-unlock-label= flag to configure via label whether a PR can be unlocked. Also tested.

[mvdkleijn]

Cool, looking good. I really didn't have time to look at this yesterday due to kid troubles. 😉 I should have more time tonight and will focus on adding a client_test.go for our client.

[mvdkleijn]

I have looked a bit into GetTeamNamesForUser() and decided that this is a low(er) priority for me currently since we would be the only client outside of the github client that implements that. I think implementing it should be possible but a bit tedious, since we would need to get the owner of the repository, verify its an organisation and than get the teams.

I agree. It's a nice to have but like you said it looked tedious. I'll probably submit a feature request to the Gitea SDK for something like GetTeamsInOrgForUser(). In any case, this would indeed be low-prio.

[florianbeisel]

I have talked to techknowlogick from the Gitea team yesterday about our integration and he offered to look into our PRs to the SDK should we need any, so if you work on that maybe give him a heads-up.

[mvdkleijn]

FYI, I created a proposal for adding this to the SDK:

func (c *Client) ListOrgTeamsForUser(org string, user string, opt ListTeamsOptions) ([]*Team, *Response, error)

See: https://gitea.com/gitea/go-sdk/issues/651

[florianbeisel]

From my perspective:

I have looked into where MergePull() is used and the only location it is getting used is in the automerger. So going over our list again I think implementation-wise we've everything done. Our tests for core functionality all went well as far as I can tell (haven't seen anything not working).

So I think we're getting close. As soon as we get gitea/client_test.go somewhat built out and maybe run another round of tests we could think about getting this in peoples hands so that we can some real-world feedback since I believe, testing every corner case of atlantis against our implementation is impossible from our end (though since we're only interested in the VCS portion most of the problems should have come up by now).

What are you thoughts on that?

[mvdkleijn]

I agree. I'll try to get a basic gitea/client_test.go finished tomorrow. Once that's done I'll do a quick once-over look to see if anything stands out that needs cleanup. Considering we both tested already, I think if the unit tests success... merge this PR into the branch for the public PR.

Then as you said, we can have other people test and fire at it.

[mvdkleijn]

@florianbeisel I'd like your opinion on something...

I started working on gitea/client_test.go and started to find myself implementing the Gitea endpoints, effectively testing the SDK.

Due to that I thought to create a mock for the SDK, causing me to create an interface and an adapter.

Honestly, I feel like that's stuff that should be done for all of the clients. At the same time, I'm starting to wonder about the added value of the gitea/client_test.go.

It strikes me as perhaps it'd be better to just present a useable / testable for others PR first and then implement the entire interface / adapter / mock / test file thing later. (mostly because I didn't really want to throw in a nonsensical test file)

[florianbeisel]

Completely agree. I just looked at the different client tests that are already done for the other clients and a lot of what is being tested here are things we get for free with the SDK. And honestly, for the tests to be really realistic, it would be better to have a real (instrumented) endpoint like a predesigned Gitea instance to test against.

So yes, I think it would be fair to at least postpone the gitea/client_test.go and possibly think about reworking the client tests entirely when there is need for it. Also I think there should at least be some buy in from the respective codeowners or the atlantis maintainers.

[mvdkleijn]

Cool, then let's go with that.

I'll do a once over this evening to see if there are any redundant comments, weird formatting, etc and do a general cleanup.

Once I'm done and you're happy to go forward, I'll merge this branch into the main PR branch and let maintainers and user know it ready for feedback.

[florianbeisel]

I've just gone over and removed some redundant or really unhelpful comments as well. Mostly some // consider handling this or that comments where I was certain the error itself would get handled by a upstream function.

Also I just looked into whether our log messages are aligned with the atlantis coding convention. Also I've basically done a once-over every part of atlantis functinality that uses a dedicated function in the client and from my side nothing stood out as obviously erroneous.

So from my side things are looking good. I think once you finish your review again we can get this merged into the PR branch and see from there.

I think it is more than likely we will get something to review on our end as soon as maintainers look into it regardless and instead of now "mulling" what more could be done before that we could very well make the jump and start fixing tangible issues when they're brought up in the PR

[mvdkleijn]

LGTM, merging into main PR branch

[florianbeisel]

I just went ahead and fixed the reported linter issues. Could you look into the issues with the tester job. TBH tests are not my strongest side :-)

For your reference:

--- FAIL: TestPost_UnsupportedVCSGithub (0.00s)
    events_controller_test.go:62: when the request is for an unsupported vcs a 400 is returned
    logger.go:130: 2024-02-17T00:58:58.093Z	DEBUG	Ignoring request since not configured to support Github
    events_controller_test.go:69: exp "Ignoring request since not configured to support GitHub" to be contained in "Ignoring request since not configured to support Github\n"
--- FAIL: TestPost_InvalidGiteaSecret (0.00s)
    events_controller_test.go:106: when the gitea payload can't be validated a 400 is returned
    logger.go:130: 2024-02-17T00:58:58.093Z	DEBUG	Ignoring request since not configured to support Gitea
    events_controller_test.go:113: exp "err" to be contained in "Ignoring request since not configured to support Gitea\n"
--- FAIL: TestPost_UnsupportedGiteaEvent (0.00s)
    events_controller_test.go:139: when the event type is an unsupported gitea event we ignore it
    logger.go:130: 2024-02-17T00:58:58.093Z	DEBUG	Ignoring request since not configured to support Gitea
    events_controller_test.go:146: exp 200 got 400, body: Ignoring request since not configured to support Gitea

[mvdkleijn]

I just went ahead and fixed the reported linter issues. Could you look into the issues with the tester job. TBH tests are not my strongest side :-)

For your reference:

--- FAIL: TestPost_UnsupportedVCSGithub (0.00s)
    events_controller_test.go:62: when the request is for an unsupported vcs a 400 is returned
    logger.go:130: 2024-02-17T00:58:58.093Z	DEBUG	Ignoring request since not configured to support Github
    events_controller_test.go:69: exp "Ignoring request since not configured to support GitHub" to be contained in "Ignoring request since not configured to support Github\n"
--- FAIL: TestPost_InvalidGiteaSecret (0.00s)
    events_controller_test.go:106: when the gitea payload can't be validated a 400 is returned
    logger.go:130: 2024-02-17T00:58:58.093Z	DEBUG	Ignoring request since not configured to support Gitea
    events_controller_test.go:113: exp "err" to be contained in "Ignoring request since not configured to support Gitea\n"
--- FAIL: TestPost_UnsupportedGiteaEvent (0.00s)
    events_controller_test.go:139: when the event type is an unsupported gitea event we ignore it
    logger.go:130: 2024-02-17T00:58:58.093Z	DEBUG	Ignoring request since not configured to support Gitea
    events_controller_test.go:146: exp 200 got 400, body: Ignoring request since not configured to support Gitea

No problem, will take a look later today or tomorrow.