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

ci: implemment pagination #266

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

ci: implemment pagination #266

wants to merge 6 commits into from

Conversation

Gmin2
Copy link

@Gmin2 Gmin2 commented Dec 17, 2023

Description
Each co-author appear in a new line and implement pagination
Related issue(s)
Resolves #263

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

TBH I am not a fan of writing this action in bash. it is hard to maintain ( I don't speak bash). 😆
can you rewrite in in almighty js?

@Gmin2
Copy link
Author

Gmin2 commented Dec 18, 2023

TBH I am not a fan of writing this action in bash. it is hard to maintain ( I don't speak bash). 😆 can you rewrite in in almighty js?

i am too not a big fan of bash but as @smoya had mentioned in the issue to use some bash thats why i have used bash but no problem i am converting it too js
cc @KhudaDad414

@smoya
Copy link
Member

smoya commented Dec 18, 2023

TBH I am not a fan of writing this action in bash. it is hard to maintain ( I don't speak bash). 😆 can you rewrite in in almighty js?

i am too not a big fan of bash but as @smoya had mentioned in the issue to use some bash thats why i have used bash but no problem i am converting it too js
cc @KhudaDad414

Totally no strong opinion on using bash or any language of our preference TBH. So 👍 on my side.

@Gmin2
Copy link
Author

Gmin2 commented Dec 18, 2023

@KhudaDad414 @smoya should i rewrite this in js or not

@derberg
Copy link
Member

derberg commented Dec 18, 2023

@Min2who please rewrite to JS as we have a lot of experience with it and it will be much better to maintain long term.

please use https://github.com/actions/github-script action that already have pagination related plugins added so writing such script will be much better DX and also much readable ❤️

so one step will be just getting data from GitHub API (by default afaik response will be stored as part of output that you will be able to access from another step https://github.com/actions/github-script?tab=readme-ov-file#reading-step-results). And in another step you will construct a commit message.

such approach will make maintenance much better + will make is easier to debug in future

@smoya
Copy link
Member

smoya commented Dec 19, 2023

i am too not a big fan of bash

As @derberg stated, +1 to use JS. From the very moment this is not gonna be a simple one liner as it was, it will become very hard to maintain.

@Gmin2
Copy link
Author

Gmin2 commented Dec 21, 2023

@KhudaDad414 @derberg @smoya can you suggest me some way to test this actions locally

@KhudaDad414
Copy link
Member

@Min2who actions are a little hard to test especially this one. I would suggest testing the script you write separately and making sure it gets what you want. testing the entire workflow is a little more challenging, you may need to test it on your fork. I think testing the script here suffices.

@Gmin2
Copy link
Author

Gmin2 commented Jan 14, 2024

i have updated this action using the github octokit.paginate plugin and have test this in my local fork action in this

cc @smoya @KhudaDad414

@Gmin2
Copy link
Author

Gmin2 commented Jan 15, 2024

made the changes

cc @KhudaDad414

KhudaDad414
KhudaDad414 previously approved these changes Jan 16, 2024
Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

LGTM! please wait for @derberg's review. 🙇

@Gmin2
Copy link
Author

Gmin2 commented Jan 21, 2024

LGTM! please wait for @derberg's review. 🙇

waiting 🙇‍♂️
cc @derberg

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@Min2who hey, left some comments how you can improve the action

can you share some example where did you test it, a test run?

.github/workflows/automerge-for-humans-merging.yml Outdated Show resolved Hide resolved
.github/workflows/automerge-for-humans-merging.yml Outdated Show resolved Hide resolved
.github/workflows/automerge-for-humans-merging.yml Outdated Show resolved Hide resolved
.github/workflows/automerge-for-humans-merging.yml Outdated Show resolved Hide resolved
.github/workflows/automerge-for-humans-merging.yml Outdated Show resolved Hide resolved
@Gmin2
Copy link
Author

Gmin2 commented Feb 28, 2024

@Min2who hey, left some comments how you can improve the action

can you share some example where did you test it, a test run?

This is my test run @derberg

@derberg
Copy link
Member

derberg commented Mar 11, 2024

@utnim2 did you see my other comments?

@Gmin2
Copy link
Author

Gmin2 commented Mar 11, 2024

@utnim2 did you see my other comments?

ahh sorry , working on it

@Gmin2
Copy link
Author

Gmin2 commented Mar 11, 2024

Done the reviewed changes

The test of this action here

cc @derberg @KhudaDad414

@Gmin2 Gmin2 requested a review from derberg March 11, 2024 21:26
@derberg
Copy link
Member

derberg commented Apr 18, 2024

@utnim2 lgtm but there were some changes done since initial implementation, can you show your changes somewhere in action, in some test repo? I can help if you need someone to open a PR in your repo, and then you make edits to make sure it works

# 5. Removes repeated authors (authors can have more than one commit in the PR).
# 6. Builds the `Co-authored-by: ...` lines with actual info.
# 7. Transforms the array into plain text. Thanks to this, the actual stdout of this step can be used by the next Workflow step (wich is basically the automerge).
cmd: |

Choose a reason for hiding this comment

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

Copy link

@alechkos alechkos left a comment

Choose a reason for hiding this comment

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

Hi @Gmin2, I added some comments that will fix issues on workflow run.

Comment on lines +46 to +51
const commitsResponse = await github.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/commits", {
owner: repository.split('/')[0],
repo: repository.split('/')[1],
pull_number: ${{ github.event.number }},
per_page: 100,
});
Copy link

Choose a reason for hiding this comment

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

Simplification and correction of the way to access the repo information:

Suggested change
const commitsResponse = await github.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/commits", {
owner: repository.split('/')[0],
repo: repository.split('/')[1],
pull_number: ${{ github.event.number }},
per_page: 100,
});
const { owner, repo } = context.repo;
const pull_number = context.issue.number;
const commitsResponse = await github.paginate("GET /repos/{owner}/{repo}/pulls/{pull_number}/commits", {
owner,
repo,
pull_number,
per_page: 100
});

Comment on lines +75 to +76

await getCoAuthors();
Copy link

Choose a reason for hiding this comment

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

Correct way to set the output in the Get List of authors step:

Suggested change
await getCoAuthors();
const coAuthors = await getCoAuthors();
core.setOutput("value", coAuthors);

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.

Fix wrong format for Co-authored automerged commits + pagination
6 participants