Skip to content
This repository has been archived by the owner on Dec 24, 2023. It is now read-only.

Replace Pulumi "role" fix with one from upstream #16

Closed
wants to merge 1 commit into from

Conversation

mmdriley
Copy link
Contributor

@mmdriley mmdriley requested a review from lukehoban January 20, 2018 07:26
Copy link

@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.

LGTM.

In general, I'd love to take updates that bring us fully up-to-date with the TF provider (as in, merge 1.7.1 release into our fork).

In this case though, we had already diverged, and this cherry-picked fix brings our divergence inline with mainline so that seems fine to do as a one-off.

BTW - Do we know if #12 is still an issue, and if so whether we need to open a bug on the TF provider for this?

@joeduffy
Copy link
Member

Is there any reason we wouldn't just merge 1.7.1 into our fork and kill two birds with one stone, while avoiding diverging and maintaining our own special mixture of cherry picked changes?

@lukehoban
Copy link

Merging in a new TF AWS provider is just more potentially disruptive.

But now's as good a time as any to take that. I'll open a PR now to just merge in 1.7.1.

@joeduffy
Copy link
Member

Great. I understand this approach more disruptive, but it's less manual work (ideally we'd never have to cherry pick), in line with where we want to go (we want to keep pushing ahead on versions), and we should be moving fast at this point (ideally not breaking things, but hey, it happens).

@ericrudder
Copy link
Member

ericrudder commented Jan 22, 2018 via email

@lukehoban
Copy link

Agreed on all points - hence why I raised this point above :-).

PR for merging in v1.7.1 on it's way now...

@lukehoban
Copy link

since we normally trade off a fix for a new set of "unknowns" ... is there
anything "special" we should do when taking a TF update?

Ideally - we would run all integration tests all the way up the stack - in pulumi-aws, pulumi-cloud, home, pulumi-ppc, pulumi-service, etc. Today, we don't have any reasonably easy way to do that up front, so our approach is just to integrate the changes quickly up the stack so that we find out about any issues within a day or so of taking a new drop (and taking corrective action if we find any issue). This is not unique to TF provider, it's the same approach as for any potentially broadly impactful changes we make in low layers of our system.

@ericrudder
Copy link
Member

ericrudder commented Jan 22, 2018 via email

@lukehoban
Copy link

#17 brought in 1.7.1, which included the fix tracked in this PR - so I'll close this one out now.

@lukehoban lukehoban closed this Jan 23, 2018
@pgavlin pgavlin deleted the upstream-roles-fix branch June 12, 2018 23:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants