feat: Add optional project_name to action #755

miguel-botelho · 2022-10-15T09:35:23Z

This PR adds a new optional property called project_name, so that the comments created by the Github action are unique, based on current stack_infra + project_name. This will facilitate working with Pulumi actions inside monorepos, so that multiple jobs are ran in a PR and each job will have its own comment, given they are for different stacks or projects.
Besides that, also added a bit more tests around the pr spec.

On another note, I went with creating project_name as optional, not sure on what is the desired approach for this one. Also, if there's any way to find the name of the Pulumi project without the user specifying on the YML, I believe would be less intrusive (but I couldn't find by reading the project's source code a way to do so). Feedback is more than welcomed!

Fixes #563

RobbieMcKinstry

Hey, @miguel-botelho thank you very much for your contribution and especially for implementing this so quickly!

if there's any way to find the name of the Pulumi project without the user specifying on the YML, I believe would be less intrusive

IMO, I think we can get the project name programmatically using the Automation API. The Project Settings have a field for the project name.

For example, we import items from the Automation API here.

You should be able to get an instance of ProjectSettings using this method. I think this method is callable whenever, but you might want to wait until after this line so the stack is selected. Forgive me, I'm not very well-versed on the Automation API.

RobbieMcKinstry · 2022-10-15T17:55:43Z

CHANGELOG.md

@@ -9,6 +9,8 @@

 - feat: Always show login information

+- feat: Add project name to avoid having unintended edited comments


Suggested change

- feat: Add project name to avoid having unintended edited comments

- feat: Includes the project name in comments, eliminating conflicts when working with multiple projects in the same repository. ([#563](https://github.com/pulumi/actions/issues/563))

Thanks for the suggestion! Committed here: 905033c

RobbieMcKinstry · 2022-10-15T18:01:03Z

src/libs/pr.ts

+  let heading = `#### :tropical_drink: \`${command}\` on ${stackName}`;
+  if (projectName) {
+    heading = `${heading} for project ${projectName}`
+  }


LGTM.

Nit: Assume projectName is set. Do you think the message would be clear enough if written as

#### :tropical_drink: \`${command}\` on ${projectName}/${stackName}`

...or do you think having for project ${projectName} is necessary for clarity?

I'm leaning towards "not clear enough" but WDYT?

I think projectName/stackName is enough IMO, if the name of the project is something indicative. Not sure what's the "norm" for Pulumi project names, but for the ones I use, the simpler output is better 😄
I ended up refactoring to be less verbose here: 905033c, but I can always change it back

RobbieMcKinstry · 2022-10-15T18:10:58Z

Oh, I also wanted to say thanks for fixing up the tests too!

miguel-botelho · 2022-10-17T10:14:38Z

Hey, @miguel-botelho thank you very much for your contribution and especially for implementing this so quickly!

if there's any way to find the name of the Pulumi project without the user specifying on the YML, I believe would be less intrusive

IMO, I think we can get the project name programmatically using the Automation API. The Project Settings have a field for the project name.

For example, we import items from the Automation API here.

You should be able to get an instance of ProjectSettings using this method. I think this method is callable whenever, but you might want to wait until after this line so the stack is selected. Forgive me, I'm not very well-versed on the Automation API.

Hey @RobbieMcKinstry , many thanks for the help and the quick review. I ended up passing the projectName as a property of the current Config object, not sure if there's a better/cleaner approach, so feel free to leave your feedback on this one: 905033c

miguel-botelho · 2022-10-17T11:06:59Z

Another question, how can I test this development on one of my projects?

simenandre

I think this looks good!

Thanks for this, I'm sure people will love this addition! Especially the ones with mono-repos with multiple stacks :)

simenandre · 2022-10-17T20:51:47Z

src/main.ts

@@ -41,6 +41,8 @@ const main = async () => {
    ? LocalWorkspace.createOrSelectStack(stackArgs, stackOpts)
    : LocalWorkspace.selectStack(stackArgs, stackOpts));

+  config.projectName = (await stack.workspace.projectSettings()).name;


Maybe we should split this into two variables?

Suggested change

config.projectName = (await stack.workspace.projectSettings()).name;

const projectSettings = await stack.workspace.projectSettings();

config.projectName = projectSettings.name;

Something like that? Just to improve readability?

Maybe we should add an invariant as well? I'm not sure if we do any validation in projectSettings(), so might not be needed.

LGTM! Just committed it here: 1f66ab8

I ended up not adding the invariant as the name property is required inside the ProjectSettings object.

RobbieMcKinstry · 2022-10-18T15:02:53Z

src/config.ts

@@ -34,6 +34,7 @@ export const config = rt
    command: command,
    stackName: rt.String,
    workDir: rt.String,
+    projectName: rt.String,


I think we can eliminate the projectName input entirely by relying on this line below:

const projectSettings = await stack.workspace.projectSettings();

We should always be able to get the project name from the config.

I removed the input part and did something similar to the isPullRequest property inside the Config object. Are you suggesting removing the projectName entirely from the Config object and pass it to the handlePullRequest method in another parameter?

Ah, did you push that change? Right now, in the diff, I see the value in Config is always set to the empty string.
My goal is to eliminate partially initialized states in the Config object. As much as possible, that Config object should be immutable:

If at the time of construction, we can access the projectName, then that works.

But if not, I'd prefer passing the projectName field into the handlePullRequest` function.

From the diff I'm seeing, the config object is initialized with an empty string here and that value is overwritten here. I'd prefer if we could avoid this mutation.

Does that help? 😅 Sorry if that's unclear or inconsistent.

Got it! I think it's easier to pass projectName as a field on the function as well (and also avoids having a variable with a different value, depending on time of calling). I'll look into it in a bit, commit the changes, and test it as well via the link you sent. I'll notify in here when I've tested it "for real" 👍

RobbieMcKinstry · 2022-10-18T17:22:44Z

Another question, how can I test this development on one of my projects?

I think you can refer to the public URL and commit SHA (or branch name).

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-versioned-actions

miguel-botelho · 2022-10-19T09:42:43Z

Hey @cobraz @RobbieMcKinstry 👋
Many thanks for the review and help getting this one figured out. I've made the necessary changes to retrieve the projectName automatically and pass it down to the handlePullRequest method. I also went ahead and compiled the project in this branch (I've already reverted it) to test it on one of our monorepos, and it looks like it's working:

Feel free to review it again and leave your feedback on this one 😄

miguel-botelho · 2022-10-28T14:57:48Z

Hey @RobbieMcKinstry , sorry for pinging you again, were you able to review this one? 🙏

Add project_name property as optional so that the comments created by the Github action are more unique, allowing monorepos with multiple projects to use the editCommentOnPr flag properly

This reverts commit eba3969.

miguel-botelho · 2022-11-09T11:38:05Z

Hey @RobbieMcKinstry , just rebased it with the most recent changes in master, it's good to take a look again 👍

RobbieMcKinstry

Thank you for this contribution! I appreciate your thoughtful effort in working through my requested changes and your patience and persistence after I went dark on review. Seriously, thank you @miguel-botelho! 🙇🏻

RobbieMcKinstry · 2022-11-10T17:00:43Z

@miguel-botelho Would you be willing to send me an email at robbie@pulumi.com? I want to send a small thank-you gift from Pulumi for your contribution.

miguel-botelho · 2022-11-10T17:36:09Z

@miguel-botelho Would you be willing to send me an email at robbie@pulumi.com? I want to send a small thank-you gift from Pulumi for your contribution.

Many thanks! I'll ping you there in a few minutes. Also, I'm the one that should be thanking for guiding me during the review phase 🙇

RobbieMcKinstry · 2022-11-10T18:18:07Z

Hah, anytime, friend!

RobbieMcKinstry suggested changes Oct 15, 2022

View reviewed changes

RobbieMcKinstry added kind/bug Some behavior is incorrect or out of spec impact/reliability Something that feels unreliable or flaky area/cicd labels Oct 15, 2022

miguel-botelho requested a review from RobbieMcKinstry October 17, 2022 10:15

simenandre approved these changes Oct 17, 2022

View reviewed changes

RobbieMcKinstry reviewed Oct 18, 2022

View reviewed changes

miguel-botelho force-pushed the feat/add-optional-project-name branch 2 times, most recently from 9d869e5 to 0df7385 Compare October 19, 2022 09:17

miguel-botelho requested review from RobbieMcKinstry and simenandre and removed request for RobbieMcKinstry October 19, 2022 09:22

miguel-botelho requested review from RobbieMcKinstry and simenandre and removed request for simenandre and RobbieMcKinstry October 19, 2022 09:51

miguel-botelho added 6 commits November 9, 2022 11:31

feat: Add optional project_name to action

284ae32

Add project_name property as optional so that the comments created by the Github action are more unique, allowing monorepos with multiple projects to use the editCommentOnPr flag properly

Refactor approach to not pass projectName via user input

1217d11

Refactor projectSettings usage

8ae9abe

Refactor projectName to pass directly to handlePullRequestMessage

96d9886

Build project in order to test it locally

ffab8aa

Revert "Build project in order to test it locally"

f0fd28e

This reverts commit eba3969.

miguel-botelho force-pushed the feat/add-optional-project-name branch from 43e3945 to f0fd28e Compare November 9, 2022 11:33

Fix rebase issues

007f11b

miguel-botelho force-pushed the feat/add-optional-project-name branch from 3dbdb02 to 007f11b Compare November 9, 2022 11:35

RobbieMcKinstry approved these changes Nov 10, 2022

View reviewed changes

RobbieMcKinstry merged commit a9fe814 into pulumi:master Nov 10, 2022

nebbles mentioned this pull request Apr 7, 2023

Commenting on PR for multiple Pulumi projects within a repo #715

Closed

simenandre mentioned this pull request Jul 18, 2023

feat: added multi-comment-support on the same PR #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add optional project_name to action #755

feat: Add optional project_name to action #755

miguel-botelho commented Oct 15, 2022

RobbieMcKinstry left a comment

RobbieMcKinstry Oct 15, 2022

miguel-botelho Oct 17, 2022 •

edited

Loading

RobbieMcKinstry Oct 15, 2022

miguel-botelho Oct 17, 2022 •

edited

Loading

RobbieMcKinstry commented Oct 15, 2022

miguel-botelho commented Oct 17, 2022

miguel-botelho commented Oct 17, 2022

simenandre left a comment

simenandre Oct 17, 2022

miguel-botelho Oct 18, 2022

RobbieMcKinstry Oct 18, 2022

miguel-botelho Oct 18, 2022

RobbieMcKinstry Oct 18, 2022

miguel-botelho Oct 18, 2022

RobbieMcKinstry commented Oct 18, 2022

miguel-botelho commented Oct 19, 2022

miguel-botelho commented Oct 28, 2022

miguel-botelho commented Nov 9, 2022

RobbieMcKinstry left a comment

RobbieMcKinstry commented Nov 10, 2022

miguel-botelho commented Nov 10, 2022

RobbieMcKinstry commented Nov 10, 2022

		@@ -9,6 +9,8 @@

		- feat: Always show login information

		- feat: Add project name to avoid having unintended edited comments

	- feat: Add project name to avoid having unintended edited comments
	- feat: Includes the project name in comments, eliminating conflicts when working with multiple projects in the same repository. ([#563](https://github.com/pulumi/actions/issues/563))

	config.projectName = (await stack.workspace.projectSettings()).name;
	const projectSettings = await stack.workspace.projectSettings();
	config.projectName = projectSettings.name;

feat: Add optional project_name to action #755

feat: Add optional project_name to action #755

Conversation

miguel-botelho commented Oct 15, 2022

RobbieMcKinstry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguel-botelho Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguel-botelho Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

RobbieMcKinstry commented Oct 15, 2022

miguel-botelho commented Oct 17, 2022

miguel-botelho commented Oct 17, 2022

simenandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbieMcKinstry commented Oct 18, 2022

miguel-botelho commented Oct 19, 2022

miguel-botelho commented Oct 28, 2022

miguel-botelho commented Nov 9, 2022

RobbieMcKinstry left a comment

Choose a reason for hiding this comment

RobbieMcKinstry commented Nov 10, 2022

miguel-botelho commented Nov 10, 2022

RobbieMcKinstry commented Nov 10, 2022

miguel-botelho Oct 17, 2022 •

edited

Loading

miguel-botelho Oct 17, 2022 •

edited

Loading