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

Adjust pipeline creation so it works for non-admins #112

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

yob
Copy link
Contributor

@yob yob commented Feb 28, 2021

For Buildkite organizations with teams enabled, it's possible for non-admins to create pipelines within their own teams.

However, it only works if the team is included in the pipelineCreate() mutation. For a non-admin graphql token, pipelineCreate() will return an error if the input includes no teams.

The existing code was calling pipelineCreate() with no teams, and then following up with a teamPipelineCreate() mutation to apply the teams. We never got to the follow up though, because the initial pipelineCreate() was failing.

This merges the two steps so the initial pipelineCreate() includes teams. Once the pipeline has been created, team management is still done independently of the rest of the pipeline. That should be fine though, it's only pipelineCreate() that requires teams to be include2. Non-admin graphql tokens can call pipelineUpdate() without teams just fine.

Still needs a fair bit of polish:

  • confirm createPipeline() still works on orgs without teams enabled
  • confirm a non-admin graphql token can call updatePipeline() with no teams included
  • acceptance tests would be really nice, if we can workout how to use a different API token in a subset of tests

Fixes #84

For Buildkite organizations with teams enabled, it's possible for
non-admins to create pipelines within their own teams.

However, it only works if the team is included in the pipelineCreate()
mutation. For a non-admin graphql token, pipelineCreate() will return an
error if the input includes no teams.

The existing code was calling pipelineCreate() with no teams, and then
following up with a teamPipelineCreate() mutation to apply the teams. We
never got to the follow up though, because the initial pipelineCreate()
was failing.

This merges the two steps so the initial pipelineCreate() includes
teams. Once the pipeline has been created, team management is still done
independently of the rest of the pipeline. That should be fine though,
it's only pipelineCreate() that requires teams to be include2.
Non-admin graphql tokens can call pipelineUpdate() without teams just
fine.

Fixes #84
@yob
Copy link
Contributor Author

yob commented Feb 28, 2021

The permutations here are a real headache, and I'd love to get some acceptance tests in place. Without tests, I'm sure we'll inadvertently break the experience for non-admin users who want to use the terraform provider.

Interestingly, in the current provider there's very few things a non-admin user can do beyond managing pipelines and pipeline schedules. The other resources (teams, agent tokens) require admin privileges, and future resources (like user) would presumably also be admin-only.

This PR was 10% working out the graphql query to include teams into pipelinesCreate(), 90% wrestling with github.com/shurcooL/graphql to get it to generate the query shape I wanted. I am very not-fond of it as a result, and would be open to replacing it without something easier to use. Or maybe someone can just show the right way to hold it and it'll all be fine.

teamsData := make([]PipelineTeamAssignmentInput, 0)
for _, team := range teamPipelines {
log.Printf("converting team slug '%s' to an ID", string(team.Team.Slug))
teamID, err := GetTeamID(string(team.Team.Slug), client)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a bit gross - for each team slug listed in the pipeline we do a graphql query to convert it into an ID. It's no worse than what we have on main though - reconcileTeamPipelines() also calls GetTeamID()

@yob yob marked this pull request as ready for review March 10, 2021 03:29
Copy link
Contributor

@chloeruka chloeruka left a comment

Choose a reason for hiding this comment

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

Paired with @yob on this, where we confirmed that the pipeline can still be created under the correct conditions (teams not enabled, teams enabled but non-admin)

@yob
Copy link
Contributor Author

yob commented Mar 10, 2021

@chloeruka and I have manually confirm pipeline creating and updating work with both admin and non-admin API tokens.

We also have a plan for acceptance testing with non-admin tokens, which we'll tackle in a follow up PR.

@yob yob merged commit 5011f8b into main Mar 10, 2021
@yob yob deleted the allow-pipeline-creation-by-non-admins branch March 10, 2021 03:31
yob added a commit that referenced this pull request Mar 10, 2021
… API token

In PR #112 we fixed the pipeline resource so it can be created and
updated by non-admin API tokens.

To prevent regressions, this adds a new acceptance test step to the
build pipeline that will run only the subset of acceptance tests that
will work with a non-admin API token.

Non-admins can manage pipelines and pipeline schedules, but only if the
pipeline belongs to a team where the user is a team maintainer.

Before this we had no acctests with teams declared on a pipeline, so
none of them would work for non-admins. This has added a couple of basic
tests that use teams.

The new tests will also run in the normal acctest step (using an admin
token), which is a nice bonus.
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.

Error adding simple pipeline. Asks for team when team is already specified.
2 participants