Skip to content

Fix location we get org from - #95

Open
hbrysiewicz wants to merge 6 commits into
necojackarc:masterfrom
soxhub:fix-org-loc
Open

Fix location we get org from#95
hbrysiewicz wants to merge 6 commits into
necojackarc:masterfrom
soxhub:fix-org-loc

Conversation

@hbrysiewicz

@hbrysiewicz hbrysiewicz commented May 23, 2023

Copy link
Copy Markdown
Contributor

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.

@hbrysiewicz
hbrysiewicz requested a review from necojackarc as a code owner May 23, 2023 17:56
@hbrysiewicz
hbrysiewicz force-pushed the fix-org-loc branch 5 times, most recently from d4cdb0c to 15d00a6 Compare May 23, 2023 22:18
@hbrysiewicz

Copy link
Copy Markdown
Contributor Author

@necojackarc I've tested this on my org repo and using it in our gh actions so it should be good now

Comment thread README.md Outdated
Comment thread 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;

Copy link
Copy Markdown
Owner

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 orgnisation
  • context.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.

Copy link
Copy Markdown
Contributor Author

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?

@hbrysiewicz
hbrysiewicz requested a review from necojackarc May 30, 2023 21:03
@hbrysiewicz

Copy link
Copy Markdown
Contributor Author

@necojackarc everything is green - just the one open question about org - only way I found was to use context.payload.repository.owner.login. Do you know of another?

@necojackarc

Copy link
Copy Markdown
Owner

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 context.payload.repository.owner.login. Have you checked what will happen if you run it in an individual account? Will it work with no errors?

@necojackarc necojackarc left a comment

Copy link
Copy Markdown
Owner

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!

Comment thread src/reviewer.js
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)) {

Copy link
Copy Markdown
Owner

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?

Comment thread src/github.js
Comment on lines +116 to +127
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);
}

Copy link
Copy Markdown
Owner

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?

Comment thread src/github.js
team_slug: team,
});

return data?.map((member) => member.login);

Copy link
Copy Markdown
Owner

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?

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.

2 participants