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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug when scope is repository #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

whywaita
Copy link
Contributor

@whywaita whywaita commented Sep 2, 2021

#11 added scope, but this change has a bug when setting the repository. 馃檱

GET /app/installations response login_name, but it is organization name or username (not repository name).
So this change fixes bugs when setting repository to scope.

@whywaita
Copy link
Contributor Author

whywaita commented Sep 2, 2021

@billyvg Hello, Please check me 馃檹

src/main.ts Outdated
return {error: `GitHub Apps can't accessible repository (${repository})`};
}

return {error: ''};
Copy link
Member

Choose a reason for hiding this comment

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

Instead of returning an object, let's throw an error. This will simplify the error handling logic where we call this function. The exception will get caught by https://github.com/getsentry/action-github-app-token/pull/17/files#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dR65

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i fixed in f318804 !

@whywaita
Copy link
Contributor Author

@billyvg Can you configure ${{ secrets.INTEGRATION_TEST_APP_ID }} in a repository? It is a reason for failure CI maybe.

@billyvg
Copy link
Member

billyvg commented Sep 13, 2021

@whywaita Hmm it should be configured -- does this work for you locally? I'm not sure, but it sounds like https://github.com/getsentry/action-github-app-token/pull/17/files#diff-4fab5baaca5c14d2de62d8d2fceef376ddddcc8e9509d86cfa5643f51b89ce3dR70-R73 needs appId?

@whywaita
Copy link
Contributor Author

whywaita commented Sep 16, 2021

@billyvg

does this work for you locally?

yes, I use the fork branch (whywaita@fix/repository-selected) to register a self-hosted runner.

Screenshot 2021-09-16 16 23 27

I will try to fix it.

@whywaita
Copy link
Contributor Author

@billyvg
hmm, I still do not understand the reason for failure 馃

I saw Actions page, I can't found with.app_id and with.private_key. Can you see it?

Screenshot 2021-09-17 14 44 16

@whywaita
Copy link
Contributor Author

whywaita commented Mar 7, 2022

I found a mistake, It needs to check pagination.
So I fixed it 25bd979.

@whywaita
Copy link
Contributor Author

@billyvg Hi, Can you re-run a CI in this branch?

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.

None yet

2 participants