Skip to content

Conversation

@sbgertp
Copy link

@sbgertp sbgertp commented May 19, 2021

sbgertp added 2 commits May 19, 2021 14:09
Use CredentialsProvider.triggeredBy to handle case when cause is UpstreamCause chain ending in UserIdCause
Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Not exactly what I had in mind originally, but this might work. I'll take a closer look at this later in the week. In the meantime, maybe @jglick has any feedback?

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

I do not understand at a conceptual level how user-scoped credentials are supposed to work, so I cannot comment on whether this is “right”. IIUC the effective change here is that UserIdCause is being considered not just on the “current” build but also on its transitive upstreams?

@jvz
Copy link
Member

jvz commented May 19, 2021

Yeah I think this might be backwards. I'd expect a build with a build step to bind its credential parameter bindings to the invoked build.

@sbgertp
Copy link
Author

sbgertp commented May 20, 2021

Yeah I think this might be backwards. I'd expect a build with a build step to bind its credential parameter bindings to the invoked build.

I agree that the binding should happen at the call site of the upstream build creating the downstream one. But changing the architecture for me is a bit much, so I framed this as a bug of the current implementation - it already performs deferred binding of credentials if directly started by the user, it was just missing the case when started by the user as part of a causal chain.

@jvz
Copy link
Member

jvz commented May 20, 2021

Oh that's interesting! Let me verify this works equivalently to what I had in mind before I can merge this. Should be within the next few days.

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.

3 participants