actions icon indicating copy to clipboard operation
actions copied to clipboard

Add `soft-refresh` config option to enable `preview --refresh`/`up --refresh`

Open fitz-vivodyne opened this issue 11 months ago • 11 comments

As described in https://github.com/pulumi/actions/issues/870, the current refresh config option runs pulumi refresh prior to the requested command, updating the state file. There is not currently a way to run pulumi preview --refresh or pulumi up --refresh.

This PR adds a soft-refresh flag which enables the --refresh flag. Not sure on the right approach for naming, but I figured this helps get across the difference between refresh updating the state and --refresh not updating the state.

Completely untested at this point. I took a look at existing tests and didn't see an obvious place to add a test. No local testing yet either, will play with that in a bit, erring on the side of opening early to enable discussion.

fitz-vivodyne avatar Mar 10 '24 16:03 fitz-vivodyne

@fitz-vivodyne Thanks for the PR and sorry about the delay here. I've just let the tests run, and there are some failures. I believe this has the same cause as https://github.com/pulumi/actions/pull/1116#pullrequestreview-1942880883, and I think the option needs to be added to the action.yml file.

tgummerer avatar Mar 18 '24 13:03 tgummerer

Thanks @tgummerer, just pushed the updates to action.yml

fitz-vivodyne avatar Mar 22 '24 01:03 fitz-vivodyne

Hello 👋 Thanks for doing this, @fitz-vivodyne!

I am wondering what you think of keep using the refresh argument, and change the behavior of it to use this?

For most users they expect to be using this way of refreshing. We need to bump major, since it might break things, but it's the same kind of breaking we've done since v2; smaller and easy to migrate.

simenandre avatar Mar 22 '24 07:03 simenandre

I am wondering what you think of keep using the refresh argument, and change the behavior of it to use this? For most users they expect to be using this way of refreshing. We need to bump major, since it might break things, but it's the same kind of breaking we've done since v2; smaller and easy to migrate.

@simenandre I found the current behavior surprising and when I first started using the action I 100% expected the refresh argument to just run with the --refresh flag. If you actually want to run a pulumi refresh I would expect to run that as a standalone action/command.

Agreed that a major version bump would be appropriate, and I'm personally supportive of fully replacing the refresh functionality.

Thoughts on moving forward? New PR, modify this one to strip out the refresh behavior?

fitz-vivodyne avatar Mar 22 '24 12:03 fitz-vivodyne

I think a new major release and changing the refresh functionality would make sense here as well. I think updating this PR sounds good, but no strong preferences here either way.

tgummerer avatar Mar 26 '24 09:03 tgummerer

+1 to changing the refresh functionality and bumping the major version. I'm guessing the reason it behaved this way originally was because automation api didn't have support for the refresh flag.

~Although, now that I think about it, I think we've only added support for --refresh for Go Automation API so far, so we'll need to port that over to the nodejs automation api to be able to be used here.~

EDIT: Looks like we at least have the refresh option for up and preview in nodejs automation api, but would be great to get this supported for destroy as well to have consistency across commands.

komalali avatar Mar 26 '24 19:03 komalali

@tgummerer I think we should provide the guidance for what we want to happen here. Let's make the decision and try to land this PR, or decide not to land it and describe the alternative.

mikhailshilkov avatar Apr 09 '24 07:04 mikhailshilkov

I'd be happy to land this PR if @fitz-vivodyne wants to update it with the updated refresh functionality. Once that's done I can approve, merge and create a new release with a major version bump.

tgummerer avatar Apr 09 '24 08:04 tgummerer

@fitz-vivodyne Will you have time to take another step on this PR?

mikhailshilkov avatar Apr 16 '24 08:04 mikhailshilkov

Sorry, haven't had bandwidth recently. Might be a while before I'm able to get around to it, anyone else can feel free to grab it if they're available

fitz-vivodyne avatar Apr 16 '24 13:04 fitz-vivodyne

@fitz-vivodyne Do you mind converting it to a Draft PR for now, so that it doesn't show up in our review queue?

mikhailshilkov avatar May 10 '24 15:05 mikhailshilkov