[go: up one dir, main page]

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 ListWorkflowJobsAttempt method to ActionsService #3060

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

fchimpan
Copy link
Contributor
@fchimpan fchimpan commented Jan 26, 2024

Copy link
codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (20fc901) 97.71% compared to head (345d70e) 97.71%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3060   +/-   ##
=======================================
  Coverage   97.71%   97.71%           
=======================================
  Files         152      152           
  Lines       13326    13342   +16     
=======================================
+ Hits        13021    13037   +16     
  Misses        215      215           
  Partials       90       90           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator
@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @fchimpan !
This looks like a great start apart from the code coverage.

I've added a suggestion that should hopefully clear up the coverage tests.

github/actions_workflow_jobs_test.go Show resolved Hide resolved
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jan 26, 2024
Copy link
Collaborator
@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @fchimpan !
LGTM

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@fchimpan
Copy link
Contributor Author

@gmlewis

Thanks for your advice. I was already working on improving the test coverage before I noticed your comments, so there may have been some overlap. I would appreciate it if you could review the changes. I look forward to your feedback.

Best regards,

@gmlewis
Copy link
Collaborator
gmlewis commented Jan 26, 2024

@fchimpan - while you are here, would you mind taking a look at other open PRs in this repo with the "Needs Review" label and reviewing them? I know that one person was waiting for #3049 in particular, but all of them need review at some point... feel free to pick and choose (or leave them alone - your choice).

@fchimpan
Copy link
Contributor Author

@gmlewis
Thank you for the comment. I understand and am happy to help.
I have an interest in this repository, so I will definitely cooperate.

Since I don't have the authority to approve pull requests, should I just leave comments on the PR if I have any suggestions?

@gmlewis
Copy link
Collaborator
gmlewis commented Jan 26, 2024

Thank you, @fchimpan !
Yes, feel free to leave comments, and when you are satisfied with the changes, you can say "LGTM" (Looks Good To Me) and I will take that as approval.

@fchimpan
Copy link
Contributor Author

Thank you @gmlewis
Understood, I will check it out.

By the way, sometimes I come across APIs that are not yet implemented. In such cases, is it alright if I propose a PR like I did this time?

Thank you in advance.

@gmlewis
Copy link
Collaborator
gmlewis commented Jan 27, 2024

By the way, sometimes I come across APIs that are not yet implemented. In such cases, is it alright if I propose a PR like I did this time?

Absolutely! We welcome all contributions.
The GitHub v3 API is amazingly huge and constantly updating, so we definitely welcome the help.
Thank you!

Copy link
Contributor
@valbeat valbeat left a comment

Choose a reason for hiding this comment

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

LGTM!

@gmlewis
Copy link
Collaborator
gmlewis commented Feb 9, 2024

Thank you, @valbeat !
Merging.

@gmlewis gmlewis merged commit 9390a86 into google:master Feb 9, 2024
7 checks passed
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Feb 9, 2024
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.

Add support for Actions list jobs for a workflow run attempt
3 participants