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

Build resolver for user's UUID #295

Merged
merged 8 commits into from
Jun 20, 2024
Merged

Conversation

lizrabuya
Copy link
Contributor

@lizrabuya lizrabuya commented Jun 20, 2024

Implement a build resolver for a given user uuid

  • Tests added

@lizrabuya lizrabuya marked this pull request as ready for review June 20, 2024 02:16
Copy link
Contributor

@jradtilbrook jradtilbrook left a comment

Choose a reason for hiding this comment

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

Looks pretty good 👍
Just a few architecture changes

internal/build/resolver/current_user.go Outdated Show resolved Hide resolved
internal/build/resolver/user_builds.go Outdated Show resolved Hide resolved
internal/build/resolver/user_builds.go Outdated Show resolved Hide resolved
"github.com/buildkite/go-buildkite/v3/buildkite"
)

func ResolveBuildFromCurrentUserId(uuid string, pipelineResolver pipelineResolver.PipelineResolverFn, f *factory.Factory) BuildResolverFn {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessarily the current users ID. It could be any user so we should remove that from the name

@lizrabuya lizrabuya requested a review from jradtilbrook June 20, 2024 03:32
Comment on lines +40 to +44
if len(builds) == 0 {
// we error here because this resolver is explicitly used so any case where it doesn't resolve a build is a
// problem
return nil, fmt.Errorf("failed to find a build for current user (%s)", userInfo)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this applies now with these other resolvers? I suppose if the user doesn't have any builds its not really an error, we could just show a helpful message without it being an error

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 guess thats right, and for consistency too as we do the same for pipelines resolver. I'll update this to not error when no builds are found

Copy link
Contributor

Choose a reason for hiding this comment

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

actually its probably more nuanced than that. the --user resolver might want to error if that user doesnt exist vs. if the user does exist but doesnt have any builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jradtilbrook , i will raise a different PR to implement this as it will have to be the same behavior for all build resolvers and ensure that this is handled as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the user does not exist, or is invalid the api returns an error, which is also returned by the resolver. So that should be covered.

@lizrabuya lizrabuya requested a review from jradtilbrook June 20, 2024 04:30
Copy link
Contributor

@jradtilbrook jradtilbrook left a comment

Choose a reason for hiding this comment

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

Looks good. @lizrabuya can you also create a card to come back and add this resolver to the build commands (its not used just yet but thats intentional)

@lizrabuya lizrabuya merged commit cb797ad into 3.x Jun 20, 2024
1 check passed
@lizrabuya lizrabuya deleted the sup-2176-resolve-builds-to-user branch June 20, 2024 05:37
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