Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add flag to specify whether to install dependencies on pulumi convert #10198

Merged
merged 3 commits into from
Jul 21, 2022
Merged

Add flag to specify whether to install dependencies on pulumi convert #10198

merged 3 commits into from
Jul 21, 2022

Conversation

aq17
Copy link
Contributor

@aq17 aq17 commented Jul 21, 2022

Description

Add install-dependencies flag to allow user to opt out of installing dependencies on pulumi convert if they only want the generated files. Defaults to `true.

Addresses #10196

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

AaronFriel
AaronFriel approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

Short and sweet.

@aq17 aq17 merged commit c7e17fe into master Jul 21, 2022
@pulumi-bot pulumi-bot deleted the aqiu/convert-dont-install-deps branch July 21, 2022 00:45
dixler
dixler reviewed Jul 21, 2022
View reviewed changes
pkg/cmd/pulumi/convert.go
@@ -128,5 +131,9 @@ func newConvertCmd() *cobra.Command {
//nolint:lll
&outDir, "out", ".", "The output directory to write the convert project to")

cmd.PersistentFlags().BoolVar(
//nolint:lll
&installDeps, "install-dependencies", true, "Whether to install dependencies specified in the pulumi project")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe consider --no-install-dependencies? since the flag should change behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

this flag might imply to the user that dependencies are not installed by default, but they are.

Copy link
Member

Choose a reason for hiding this comment

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

Yeh I feel this should of copied 'new' https://github.com/pulumi/pulumi/blob/master/pkg/cmd/pulumi/new.go#L483 and be 'generate-only'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dixler @Frassle agreed, will update 😃

@aq17 aq17 restored the aqiu/convert-dont-install-deps branch July 21, 2022 16:24
@aq17 aq17 deleted the aqiu/convert-dont-install-deps branch July 21, 2022 16:26
@aq17 aq17 restored the aqiu/convert-dont-install-deps branch July 21, 2022 16:27
@aq17 aq17 mentioned this pull request Jul 21, 2022
Merged
3 tasks
@mikhailshilkov
Copy link
Member

mikhailshilkov commented Jul 22, 2022 •
edited
Loading

You may know it - but just in case: the text Addresses https://github.com/pulumi/pulumi/issues/10196 doesn't automatically close the linked issue, while Fixes https://github.com/pulumi/pulumi/issues/10196 (or Fixes #10196) would.

@aq17
Copy link
Contributor Author

aq17 commented Jul 22, 2022

You may know it - but just in case: the text Addresses https://github.com/pulumi/pulumi/issues/10196 doesn't automatically close the linked issue, while Fixes https://github.com/pulumi/pulumi/issues/10196 (or Fixes #10196) would.

I did not know this before 🙌🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dixler dixler dixler left review comments

@AaronFriel AaronFriel AaronFriel approved these changes

@Frassle Frassle Awaiting requested review from Frassle

@iwahbe iwahbe Awaiting requested review from iwahbe

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aq17 @mikhailshilkov @AaronFriel @Frassle @dixler