-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix location we get org from #95
base: master
Are you sure you want to change the base?
Conversation
d4cdb0c
to
15d00a6
Compare
@necojackarc I've tested this on my org repo and using it in our gh actions so it should be good now |
src/github.js
Outdated
async function get_team_members(team) { | ||
const context = get_context(); | ||
const octokit = get_octokit(); | ||
const org = core.getInput('org') || context.payload.repository.owner.login; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate why we'd like to have this logic? If I'm correct,
core.getInput('org')
allows us to pas an orgnisationcontext.payload.repository.owner.login
returns a username, not org name
Also, if we'd like to add a new input parameter, we should update the README, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I misunderstood the use of core
and can remove that bit - we shouldn't need a new input.
context.payload.repository.owner.login
returns the organization if the repo owner is an organization. If it returns a username, then this action would error out but afaict there is no way to get just the organization name for a repository or to tell the difference between an org or username and leave a useful error message.
Do you know of a way?
@necojackarc everything is green - just the one open question about org - only way I found was to use |
FYI - I'm on holiday right now. Will get back to the PR sometime next week when I'm back. @hbrysiewicz Honestly, I don't have any other ideas other than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for waiting. I'm finally back here and got some time to check my personal GitHub account... I put some comments but it looks almost good to go to me 👍 Thanks a lot!
const individuals_in_author_setting = replace_groups_with_individuals({ reviewers: [ author ], config }); | ||
|
||
if (individuals_in_author_setting.includes(specified_author)) { | ||
if (individuals_in_author_setting?.includes(specified_author)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you need to add ?
? Is your changing adding something that can make individuals_in_author_setting
null
or undefined
?
async function get_team_members(team) { | ||
const context = get_context(); | ||
const octokit = get_octokit(); | ||
const org = context.payload.repository.owner.login; | ||
|
||
const { data } = await octokit.teams.listMembersInOrg({ | ||
org, | ||
team_slug: team, | ||
}); | ||
|
||
return data?.map((member) => member.login); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will it happen if you call this function in a repository under an individual account?
team_slug: team, | ||
}); | ||
|
||
return data?.map((member) => member.login); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If data
can't be null
or undefined
, I wouldn't add ?
since it can hide some unexpected errors. In what case can data
be null
or undefined
?
Location off context that was being used for organization was not correct. Once fixed I uncovered some other issues.
First time working on GH action so wasn't sure how to test properly initially but this branch is now working and running as expected.