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: gitness/harness prebuild webhook #1199

Merged
merged 15 commits into from
Oct 21, 2024

Conversation

the-johnwick
Copy link
Contributor

@the-johnwick the-johnwick commented Oct 4, 2024

gitness/harness prebuild webhook

Description

This PR add Prebuild hook for gitness/harness.
/claim #997

ee.mp4

@the-johnwick the-johnwick force-pushed the gitness-prebuild-webhook branch 4 times, most recently from 35c4f09 to 979f99a Compare October 7, 2024 20:30
@the-johnwick the-johnwick marked this pull request as ready for review October 7, 2024 20:30
@the-johnwick the-johnwick requested a review from a team as a code owner October 7, 2024 20:30
@the-johnwick the-johnwick marked this pull request as draft October 8, 2024 05:21
@the-johnwick the-johnwick marked this pull request as ready for review October 8, 2024 08:06
@the-johnwick the-johnwick changed the title feat: gitness/harness prebuild webhook (WIP) feat: gitness/harness prebuild webhook Oct 8, 2024
pkg/gitprovider/gitness.go Outdated Show resolved Hide resolved
pkg/gitprovider/gitnessclient/gitness_client.go Outdated Show resolved Hide resolved
Copy link
Member

@Tpuljak Tpuljak left a comment

Choose a reason for hiding this comment

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

I'll move the PR to draft until you address the comment below. Please provide a video where you test the PR with commit interval and trigger files.

pkg/gitprovider/gitness.go Show resolved Hide resolved
@Tpuljak Tpuljak marked this pull request as draft October 8, 2024 12:21
@the-johnwick the-johnwick force-pushed the gitness-prebuild-webhook branch 2 times, most recently from b1f7c4d to 33b369a Compare October 8, 2024 19:51
@the-johnwick
Copy link
Contributor Author

done.mp4

pkg/gitprovider/gitness.go Show resolved Hide resolved
@the-johnwick
Copy link
Contributor Author

@Tpuljak Gitness api for listing all commits has param called after we can pass hash to that param and it will return the commit after that hash. So we don't need to list all commit and filter it.

@Tpuljak
Copy link
Member

Tpuljak commented Oct 9, 2024

@Tpuljak Gitness api for listing all commits has param called after we can pass hash to that param and it will return the commit after that hash. So we don't need to list all commit and filter it.

But how are you supposed to know how many commits are made between initialSha and currentSha if you don't even use currentSha in the function 😄

Forget the prebuild trigger for now, just focus on GetCommitsRange and make it return the number of commits between initialSha and currentSha

@the-johnwick
Copy link
Contributor Author

@Tpuljak Gitness api for listing all commits has param called after we can pass hash to that param and it will return the commit after that hash. So we don't need to list all commit and filter it.

But how are you supposed to know how many commits are made between initialSha and currentSha if you don't even use currentSha in the function 😄

Forget the prebuild trigger for now, just focus on GetCommitsRange and make it return the number of commits between initialSha and currentSha

@Tpuljak I think currentSha means the latest one. Right?

@Tpuljak
Copy link
Member

Tpuljak commented Oct 9, 2024

@Tpuljak I think currentSha means the latest one. Right?

It means exactly that sha. It might be the latest one but it might not be. It's not important if it's the last one of not.

@the-johnwick
Copy link
Contributor Author

@Tpuljak oh I thought it will be latest one every time. My bad.

@the-johnwick
Copy link
Contributor Author

the-johnwick commented Oct 9, 2024

@Tpuljak Updated the changes. Please Take a look I will upload the video soon

@the-johnwick
Copy link
Contributor Author

@Tpuljak

clideo_editor_5970bf36aec245e193096b90f9d59372.mp4

@idagelic
Copy link
Member

image

@the-johnwick I just tried adding a Gitness provider after cleaning up the Daytona config and it seems that the base API url ends up empty in the database? Can you check once more on your side? Workspace creation fails with 500 as a result

@the-johnwick
Copy link
Contributor Author

@idagelic Sure I will check whats happening.

Signed-off-by: the-johnwick <[email protected]>
@the-johnwick
Copy link
Contributor Author

@idagelic Solved this issue. We were not setting it while listing.

@the-johnwick
Copy link
Contributor Author

@idagelic Apologies for the delayed commit. I accidentally pushed the commit to my fork's main when I meant to tag you. I just noticed it and have now committed the changes to this PR.

@the-johnwick
Copy link
Contributor Author

Hi @idagelic, I just wanted to kindly check in on the PR status and see if there's any feedback when you have a moment. Thanks in advance! 🙏

Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

This doesn't work for url's containing capital letters because you didn't clean up the repository url. Attach another video when fixed

pkg/gitprovider/gitness.go Outdated Show resolved Hide resolved
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Signed-off-by: the-johnwick <[email protected]>
Copy link
Member

@idagelic idagelic left a comment

Choose a reason for hiding this comment

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

Nice work

@the-johnwick
Copy link
Contributor Author

Nice work

Thank You so much 😊

@Tpuljak Tpuljak merged commit 5ce468c into daytonaio:main Oct 21, 2024
12 checks passed
@the-johnwick the-johnwick deleted the gitness-prebuild-webhook branch October 21, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants