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

Expose GitHub team ID #153

Merged
merged 10 commits into from
Jul 19, 2023
Merged

Expose GitHub team ID #153

merged 10 commits into from
Jul 19, 2023

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Jul 17, 2023

The team creation API for GitHub teams only requires a githubTeamID parameter, which it uses to fetch all the other details like team name, description, members, etc.

I implemented Read while I was in here, because it was surprising to see this break refresh due to a not-implemented error. Also implemented Check because name and githubTeamId are only required for certain team types.

Tested and confirmed working by with the following program:

import * as pulumi from "@pulumi/pulumi";
import * as service from "@pulumi/pulumiservice";

const team = new service.Team("github-team", {
  organizationName: "pulumi",
  teamType: "github",
  githubTeamId: 7475955,
});
❯ pulumi up -s pulumi/test
Previewing update (pulumi/test)

View in Browser (Ctrl+O): https://.../pulumi/pulumi-service-teams-example-ts/test/previews/3dd36e22-e7d7-4b5d-82d0-2bbd8d61f86e

     Type                         Name                                  Plan
     pulumi:pulumi:Stack          pulumi-service-teams-example-ts-test
 +   └─ pulumiservice:index:Team  github-team                           create


Resources:
    + 1 to create
    1 unchanged

Do you want to perform this update? details
  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:test::pulumi-service-teams-example-ts::pulumi:pulumi:Stack::pulumi-service-teams-example-ts-test]
    + pulumiservice:index:Team: (create)
        [urn=urn:pulumi:test::pulumi-service-teams-example-ts::pulumiservice:index:Team::github-team]
        [provider=urn:pulumi:test::pulumi-service-teams-example-ts::pulumi:providers:pulumiservice::default_0_9_1_alpha_1689717595_28bbd1fc_dirty::7a11ee03-f53f-4ebc-8c9f-a09cfe4ad07a]
        githubTeamId    : 7475955
        organizationName: "pulumi"
        teamType        : "github"

Do you want to perform this update? yes
Updating (pulumi/test)

View in Browser (Ctrl+O): https://.../pulumi/pulumi-service-teams-example-ts/test/updates/5

     Type                         Name                                  Status
     pulumi:pulumi:Stack          pulumi-service-teams-example-ts-test
 +   └─ pulumiservice:index:Team  github-team                           created (0.94s)


Resources:
    + 1 created
    1 unchanged

Duration: 3s

Fixes #151.

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

I believe some additional changes to the schema will be needed. Also would be great to add test coverage for this - though not sure how easy it will be to automate that end-to-end. At the very least - a test that runs a preview to ensure this is supported correctly in the generated SDK.

},
"githubTeamID": {
"description": "The GitHub ID of the team to mirror. This is the only required parameter when creating a GitHub team -- all other parameters are taken from GitHub directly. Must be in the same GitHub organization that the Pulumi org is backed by.",
"type": "number"
Copy link
Member

Choose a reason for hiding this comment

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

This needs to also be listed as an input property, right?

The schema still marks the other input properties as required, do those need to be made optional now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The schema still marks the other input properties as required, do those need to be made optional now?

Updated so name is now optional, org and type are still needed to construct relevant URLs.

Copy link
Member

Choose a reason for hiding this comment

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

Do we validate the various combinations in Check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgavlin doesn't seem like it https://github.com/pulumi/pulumi-pulumiservice/blob/main/provider/pkg/provider/team.go#L99-L101

Most of this provider's checks seem to no-op like this. If it's worth doing while we're in here I can implement it.

Copy link
Member

Choose a reason for hiding this comment

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

Most of this provider's checks seem to no-op like this

:(

If it's worth doing while we're in here I can implement it.

I think that it's worth doing in this case b/c there are different combinations of valid inputs depending on the team type.

Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about Check

@blampe blampe merged commit dd5e1dd into main Jul 19, 2023
12 checks passed
@blampe blampe deleted the teamid branch July 19, 2023 22:13
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.

GitHub Managed Team creation fails with '400 API error: Bad Request: No TeamID provided'
3 participants