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

Add a function to get the latest commit #4

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

ttetzlaff
Copy link
Contributor

No description provided.

Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

Good work!
Please look at my inline comments.
Also - I wonder whether Commit ID is the best naming. What do you think about Commit Hash?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
vcsclient/bitbucketcloud.go Outdated Show resolved Hide resolved
vcsclient/bitbucketcloud_test.go Outdated Show resolved Hide resolved
vcsclient/bitbucketcloud.go Outdated Show resolved Hide resolved
vcsclient/bitbucketserver_test.go Outdated Show resolved Hide resolved
vcsclient/github_test.go Outdated Show resolved Hide resolved
vcsclient/bitbucketserver.go Outdated Show resolved Hide resolved
vcsclient/bitbucketcloud.go Show resolved Hide resolved
vcsclient/bitbucketcloud.go Outdated Show resolved Hide resolved
vcsclient/bitbucketserver_test.go Outdated Show resolved Hide resolved
vcsclient/github.go Outdated Show resolved Hide resolved
vcsclient/github.go Outdated Show resolved Hide resolved
vcsclient/vcsclient.go Outdated Show resolved Hide resolved
vcsclient/bitbucketcloud.go Outdated Show resolved Hide resolved
vcsclient/bitbucketserver_test.go Outdated Show resolved Hide resolved
vcsclient/gitlab_test.go Show resolved Hide resolved
@cyrilc-pro
Copy link
Contributor

cyrilc-pro commented Oct 22, 2021

@yahavi Would it make sense to not just return the commit hash but the full details (hash, author, comment) ? We could event return the N latest commits.
@ttetzlaff Do you know if it is doable?

@yahavi
Copy link
Member

yahavi commented Oct 22, 2021

@yahavi Would it make sense to not just return the commit hash but the full details (hash, author, comment)?

Yes, of course. You can return a struct. We already return a struct in ParseIncomingWebhook.

We could event return the N latest commits.

We can, but do we need more than 1 commit?

@ttetzlaff ttetzlaff force-pushed the get-latest-commit branch 2 times, most recently from d4659b1 to e6a2870 Compare October 22, 2021 10:52
@ttetzlaff
Copy link
Contributor Author

ttetzlaff commented Oct 22, 2021

Would it make sense to not just return the commit hash but the full details (hash, author, comment) ? We could event return the N latest commits.

I thought about this too, but there are many differences between the providers and I wasn't sure what to return. We could return a struct (or slice of said struct) and add fields later if needed, but I'm not sure how froggit handles api versioning. @yahavi What do you think?

@yahavi
Copy link
Member

yahavi commented Oct 22, 2021

@ttetzlaff take a look at the WebhookInfo struct. It is generic enough for all VCS providers.
You can create CommitInfo struct which may contain the commit hash, commit message, commit time, and more.

@ttetzlaff ttetzlaff force-pushed the get-latest-commit branch 3 times, most recently from eeb6bd5 to 5a7912e Compare October 25, 2021 09:56
@ttetzlaff ttetzlaff requested a review from yahavi October 26, 2021 11:59
@ttetzlaff ttetzlaff changed the title Add a function to get the latest commit id Add a function to get the latest commit Oct 26, 2021
vcsclient/bitbucketcloud.go Outdated Show resolved Hide resolved
vcsclient/vcsclient.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yahavi yahavi merged commit 6df450b into jfrog:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants