Support install-only workflow. #834

RobbieMcKinstry · 2023-01-13T20:50:09Z

This commit adds an install command option which halts execution after the Pulumi CLI is installed. This attempts to replicate the @pulumi/setup-pulumi action. To test this, most logic in main() was moved to a separate file that can be imported in a test context, unlike main.ts, which executes main() during resolution.

Fixes #686

README.md

Frassle · 2023-01-23T16:43:45Z

src/__tests__/run.test.ts

@@ -1,7 +1,53 @@
 import * as pulumiCli from "../libs/pulumi-cli";
 import { login } from '../login';
+// import * as loginModule from '../login';


src/run.ts

RobbieMcKinstry · 2023-02-13T20:34:37Z

@Frassle I still need to rebase but otherwise I'm ready for another review. This commit message explains how I embraced your suggestion and the design constraints on this changeset.

pulumi-bot · 2023-03-08T18:38:04Z

Hi 👋

Thank you for this contribution!
Due to security reasons, we only allow updating the dist/** files with our own workflows.

If the tests use the pulumi installed on the GitHub workers, then they can potentially mask bugs in the action's install step, which is what happened in #834.

abhinav · 2023-03-08T19:49:59Z

Current dependencies on/for this PR:

master
- PR ci/test: Don't use pre-installed Pulumi #877
  - PR Support install-only workflow. #834 👈

This comment was auto-generated by Graphite.

abhinav · 2023-03-08T19:52:48Z

Updates:

Undid the refactor moving code to run.ts because that made it difficult to review the changes.
Fixed a bug where the CLI was not installed for regular run commands. The job passed for all but macOS because of that bug previously because macOS workers don't have pulumi installed on workers by default. Created ci/test: Don't use pre-installed Pulumi #877 to prevent this in the future.
Added a test to verify the install-only workflow.
Updated README to reflect new changes.
Updated action.yml to mark 'command' and 'stack' optional

If the tests use the pulumi installed on the GitHub workers, then they can potentially mask bugs in the action's install step, which is what happened in #834.

This commit adds support for an install-only workflow. If command is not specified, this will halt execution after the CLI is installed. This attempts to replicate the @pulumi/setup-pulumi action. To test this, most logic in main() was moved to a separate file which can be imported in a test context, unlike main.ts, which executes main() during resolution.

Adds a test to verify that the action can run in install-only mode.

Makes reviewing this PR easier by undoing the refactor.

The job was broken because in adding support for install-only we neglected to run the install in the run case.

Marks `command` and `stack-name` as optional. Updates guidance in README about install-only mode.

There's something smelly about using the ValidationError to decide whether we should run in install-only or action mode. Switch to a plain value returned by runtimes.validate (it has a success: bool field) and use that to decide if we're in install-only or full mode. This gets rid of the extraneous `else throw` as well, because the install-only mode will never throw an exception.

justinvp

I'm not familiar with writing actions or this codebase, but I read over the change and it generally LGTM, aside from some minor questions.

justinvp · 2023-03-09T01:13:25Z

CHANGELOG.md

+- feat: Add support for an `install` command, similar to
+  [setup-pulumi](https://github.com/marketplace/actions/setup-pulumi)


Why is this listed under 3.2.1 and not HEAD?

Ah, probably because there was a release since this change was made and git merge decided to put it in the old position. Good catch.

justinvp · 2023-03-09T01:13:36Z

CHANGELOG.md

+- feat: Add support for an `install` command, similar to
+  [setup-pulumi](https://github.com/marketplace/actions/setup-pulumi)
+
+--


What's with this trailing --?

Not sure why this was added. Will fix.
(Somehow I didn't look over the changelog thoroughly.)

Figured out where the -- came from.
The -- separates the unreleased section from the rest. (https://github.com/pulumi/actions/blob/master/CONTRIBUTING.md)
However, because the original changelog entry was added below a then-unreleased entry,
when git merged these changes with master, it placed this entry under the original unreleased entry.

justinvp · 2023-03-09T01:18:38Z

src/__tests__/run.test.ts

@@ -3,6 +3,53 @@ import { login } from '../login';

 const spy = jest.spyOn(pulumiCli, 'run');

+const installConfig: Record<string, string> = {
+  command: undefined,
+  pulumiVersion: "^4",


Why is this ^4 vs. ^3?

Likely because the test wanted to verify with a non-default value.
However, I also see that this test doesn't actually verify the result at all.
I'm going to use something older (^2?) here and update the test to verify that the non-default value is picked up.

The test only verifies the truthiness of the value. Change it to verify the actual result. This also caught a bug in the test: It was using `pulumiVersion`, not `pulumi-version`, which is the name of the actual input.

RobbieMcKinstry requested review from abhinav, simenandre and Zaid-Ajaj January 13, 2023 20:50

RobbieMcKinstry requested a review from a team January 23, 2023 16:27

Frassle reviewed Jan 23, 2023

View reviewed changes

RobbieMcKinstry requested a review from Frassle February 13, 2023 20:34

abhinav force-pushed the mckinstry/install-only branch 3 times, most recently from 28bed6a to ea6cbc9 Compare March 8, 2023 19:17

abhinav added a commit that referenced this pull request Mar 8, 2023

ci/test: Don't use pre-installed Pulumi

9af94ab

If the tests use the pulumi installed on the GitHub workers, then they can potentially mask bugs in the action's install step, which is what happened in #834.

abhinav mentioned this pull request Mar 8, 2023

ci/test: Don't use pre-installed Pulumi #877

Merged

ringods mentioned this pull request Mar 8, 2023

Support an Installation-Only Workflow #686

Closed

abhinav changed the base branch from master to abhinav/ci-remove-pulumi March 8, 2023 19:49

abhinav force-pushed the mckinstry/install-only branch from b6a5df2 to 49674db Compare March 8, 2023 19:49

abhinav requested a review from a team March 8, 2023 19:53

Base automatically changed from abhinav/ci-remove-pulumi to master March 9, 2023 01:14

abhinav added a commit that referenced this pull request Mar 9, 2023

ci/test: Don't use pre-installed Pulumi (#877)

3a600b6

If the tests use the pulumi installed on the GitHub workers, then they can potentially mask bugs in the action's install step, which is what happened in #834.

RobbieMcKinstry and others added 7 commits March 8, 2023 17:14

Update README and CHANGELOG to reflect addition of install command.

e8e7891

ci: Add test for install only

b7a50e3

Adds a test to verify that the action can run in install-only mode.

Move changes back to main.ts

43b01f2

Makes reviewing this PR easier by undoing the refactor.

run: Fix installation

4b05e24

The job was broken because in adding support for install-only we neglected to run the install in the run case.

doc: Update README and action.yml

a5ab666

Marks `command` and `stack-name` as optional. Updates guidance in README about install-only mode.

abhinav force-pushed the mckinstry/install-only branch from 974ad32 to bb28bd2 Compare March 9, 2023 01:15

justinvp approved these changes Mar 9, 2023

View reviewed changes

abhinav added 2 commits March 8, 2023 17:25

test: Verify non-default pulumi-version is picked up

eecf166

The test only verifies the truthiness of the value. Change it to verify the actual result. This also caught a bug in the test: It was using `pulumiVersion`, not `pulumi-version`, which is the name of the actual input.

CHANGELOG: Move entry to the right section

400815c

justinvp approved these changes Mar 9, 2023

View reviewed changes

abhinav merged commit 3ace4a6 into master Mar 9, 2023

abhinav deleted the mckinstry/install-only branch March 9, 2023 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support install-only workflow. #834

Support install-only workflow. #834

RobbieMcKinstry commented Jan 13, 2023

Frassle Jan 23, 2023

RobbieMcKinstry commented Feb 13, 2023

pulumi-bot commented Mar 8, 2023 •

edited

Loading

abhinav commented Mar 8, 2023

abhinav commented Mar 8, 2023

justinvp left a comment

justinvp Mar 9, 2023

abhinav Mar 9, 2023

justinvp Mar 9, 2023

abhinav Mar 9, 2023

abhinav Mar 9, 2023

justinvp Mar 9, 2023

abhinav Mar 9, 2023 •

edited

Loading

		- feat: Add support for an `install` command, similar to
		[setup-pulumi](https://github.com/marketplace/actions/setup-pulumi)

Support install-only workflow. #834

Support install-only workflow. #834

Conversation

RobbieMcKinstry commented Jan 13, 2023

Frassle Jan 23, 2023

Choose a reason for hiding this comment

RobbieMcKinstry commented Feb 13, 2023

pulumi-bot commented Mar 8, 2023 • edited Loading

abhinav commented Mar 8, 2023

abhinav commented Mar 8, 2023

justinvp left a comment

Choose a reason for hiding this comment

justinvp Mar 9, 2023

Choose a reason for hiding this comment

abhinav Mar 9, 2023

Choose a reason for hiding this comment

justinvp Mar 9, 2023

Choose a reason for hiding this comment

abhinav Mar 9, 2023

Choose a reason for hiding this comment

abhinav Mar 9, 2023

Choose a reason for hiding this comment

justinvp Mar 9, 2023

Choose a reason for hiding this comment

abhinav Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

pulumi-bot commented Mar 8, 2023 •

edited

Loading

abhinav Mar 9, 2023 •

edited

Loading