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

feat: Add Gitea support #4229

Merged
merged 61 commits into from
Mar 18, 2024
Merged

Conversation

mvdkleijn
Copy link
Contributor

@mvdkleijn mvdkleijn commented Feb 11, 2024

what

Addition of Gitea client in order to close #3538

A functionally complete implementation of a client for Gitea, with the sole exception being the listing of teams that a user is a member of. However, that's not uncommon for other clients.

It was tested manually against both a Gitea and a Forgejo install. Automated tests were limited to the basics. A large part of the implementation uses the Gitea SDK.

why

See #3538

references

testing

  • TestExecute_ValidateVCSConfig
  • TestExecute_GiteaUser
  • TestExecute_GiteaBaseURLScheme
  • TestExecute_GiteaWithWebhookSecret
  • TestExecute_GiteaBaseURLPort

Also: manual testing by @florianbeisel and @mvdkleijn against Gitea and Forgejo

@mvdkleijn mvdkleijn changed the title Add Gitea support [WIP] - Add Gitea support Feb 11, 2024
@mvdkleijn mvdkleijn changed the title [WIP] - Add Gitea support feat: Add Gitea support Feb 11, 2024
@mvdkleijn mvdkleijn mentioned this pull request Feb 11, 2024
1 task
@mvdkleijn
Copy link
Contributor Author

Small update for those keeping track: @florianbeisel found my PR and had also started work on his own version for this issue. After contacting me, we decided to work together on this PR.

mvdkleijn and others added 15 commits February 13, 2024 22:52
This changes adds support for Gitea Webhook Signatures by wrapping the
function from the Gitea SDK and calling it from `handleGiteaPost()`.
1.22 as in the previous go.mod is a development version. When referencing
a minimum release version the correct format is 1.22.0
Apparently there's no max comment length in Gitea at this point in time.
Integration of mvdkleijn/atlantis and florianbeisel/atlantis efforts
@mvdkleijn mvdkleijn marked this pull request as ready for review February 16, 2024 22:39
@mvdkleijn mvdkleijn requested review from a team as code owners February 16, 2024 22:39
@mvdkleijn mvdkleijn removed the request for review from a team February 16, 2024 22:39
@X-Guardian
Copy link
Contributor

Yes, same pattern as the Atlantis GitHub and GitLab clients. Take a look at them.

@mvdkleijn
Copy link
Contributor Author

Hmmm... I don't really like the idea of putting the Gitea REST endpoints in debug logging statements which I believe is what you're suggesting. That would mean having to update the statements anytime the API changes right? I feel like that kind of defeats the purpose of using an SDK.

What would be an option... I could check if logger is set to Debug level and in that case use the SDK's SetDebugMode() function? That would put the SDK in debug level?

@X-Guardian
Copy link
Contributor

The 'SDK' is just a thin wrapper around the Gitea API, exactly the same as the GitLab and GitHub Go packages used by Atlantis. The endpoints will be fixed. When there are issues, without a way of seeing the API return codes, it will be very difficult to work out what is happening. We have been through this pain with GitHub and GitLab.

The verbose logging within Atlantis commands relies on debug lines being written to the logger irrelevant of whether the server is running with the debug log-level.

@icyleaf
Copy link

icyleaf commented Mar 2, 2024

Perhaps use the SetDebugMode method in the Gitea SDK to enable debug mode directly?

@mvdkleijn
Copy link
Contributor Author

That's what I suggested earlier but that's apparently not acceptable. I'll revisit this either tonight or tomorrow after I'm done pulling cables.

@mvdkleijn
Copy link
Contributor Author

@X-Guardian I still do not agree, but apparently the recommendation is mandatory. I added the requested debug statements.

@mvdkleijn
Copy link
Contributor Author

Hey all / @nitrocode is this still waiting for / need something?

@nitrocode
Copy link
Member

Hi @mvdkleijn. Please fix the merge conflict and request a review from x-guardian.

@mvdkleijn
Copy link
Contributor Author

@X-Guardian I've added the requested logging, was there anything else that stood out? I'll fix the merge conflict (only small conflict in go.sum) tonight when I have proper access.

@florianbeisel
Copy link
Contributor

@mvdkleijn: the merge conflict should be fixed now

Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GenPage
Copy link
Member

GenPage commented Mar 16, 2024

I will finish up the review tonight and coordinate with @X-Guardian

@GenPage
Copy link
Member

GenPage commented Mar 17, 2024

@X-Guardian I still do not agree, but apparently the recommendation is mandatory. I added the requested debug statements.

To clarify, when you use the Atlantis commands in a PR comment, you can pass a "verbose" flag that will append all logging output to the comment response. This includes log events that are debug level, even if the Atlantis server configuration is not set to debug.

It also appears that SetDebugMode doesn't log messages but rather prints to standard output with fmt.Printf. This would prevent Atlantis from being able to capture those messages and is why SetDebugMode would be an insufficient requirement. @X-Guardian was correct into requiring the additional debug logging in the application implementation.

Otherwise, the code looks good! Thank you both for the time/effort to implement support for Gitea. The community will greatly benefit and appreciate the support.

@GenPage GenPage enabled auto-merge (squash) March 17, 2024 20:44
@GenPage GenPage removed waiting-on-review Waiting for a review from a maintainer do-not-merge labels Mar 17, 2024
@florianbeisel
Copy link
Contributor

Thanks @GenPage and @X-Guardian for the context on the debug statements and where they are being used in practice. This has cleared up a few things for me. I don't want to speak for @mvdkleijn but I still would like to give some final context - though I believe we're now all on the same page here.

I initially had reservations about the approach to debug logging. Our decision to rely on the SetDebugMode function was based on our understanding and discussions at the time. However, I acknowledge that at least my knowledge was incomplete, especially regarding the verbose flag.

For me implementing support for Gitea was a significant effort and my biggest contribution to any project really and I am glad to have been a part of it. Y'alls feedback has been invaluable in steering this PR in the right direction (or keeping it on track).

So thank you again for creating a welcoming atmosphere and seeing this through to completion. I will be looking forward to contribute tp the project again should something come up

@mvdkleijn
Copy link
Contributor Author

mvdkleijn commented Mar 18, 2024

Hi there @GenPage , @X-Guardian , @nitrocode and others,

I'd like to echo @florianbeisel 's sentiment with regards to the friendly reception we received when working on this PR.

One aspect I appreciate for example is that the PR did not "hang in limbo" too long as it were, since the longer a PR takes to get reviewed, the more difficult it is to manage for the creator in my personal experience.

The requirement for and clarification of the logging certainly helps with understanding and as such is definitely appreciated. Sometimes one can get too focused on specific parts too keep an eye on the total.

Though on occasion I had to find time in between family life due to stuff, as no doubt goes for others here too... I feel happy that we could contribute this to the community.

I'd also like to say that working together with @florianbeisel was a very nice experience from my point of view.

For example: we managed to more quickly respond to feedback because of the co-op when one of us was temporarily occupied. The co-op was much appreciated; thanks for the experience Florian.

Like Florian I fully intend to support this code and (time permitting) work on other code as well.

Though maybe I'll restrict myself to before 00:00 at night next time Florian. (^_^)

@GenPage GenPage merged commit 722c4a9 into runatlantis:main Mar 18, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies PRs that update a dependency file docs Documentation feature New functionality/enhancement go Pull requests that update Go code provider/gitea
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Support Forgejo / Gitea webhooks
10 participants