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

Don't yarn link @pulumi dependencies #467

Merged
merged 1 commit into from
May 4, 2018
Merged

Don't yarn link @pulumi dependencies #467

merged 1 commit into from
May 4, 2018

Conversation

ellismg
Copy link
Contributor

@ellismg ellismg commented Apr 26, 2018

Just opening this so I can get CI running on it and iterate.

@ellismg ellismg force-pushed the ellismg/no-link branch 9 times, most recently from 742183f to bab54ff Compare April 26, 2018 21:30
@ellismg
Copy link
Contributor Author

ellismg commented Apr 26, 2018

This works as is, but we still link @pulumi/cloud and @pulumi/cloud-aws in our examples.

Talking with Luke, we might consider doing an yarn pack to create a tarball after building our packages and then yarn add these tarballs in our integration test framework instead of linking. This would ensure our node_modules folder looks more like it would in a customer application (instead of having symlinks to other random folders).

@ellismg
Copy link
Contributor Author

ellismg commented Apr 30, 2018

Let's stick with linking for now (since that aligns with the way folks will do development on these packages) and if it turns out that in CI we want to try to publish and install a local version of the package, we can do that.

@ellismg
Don't yarn link @pulumi dependencies
b385733 
Instead of downloading versions of `@pulumi/pulumi` and `@pulumi/aws`,
extracting them and then running `yarn link`, just use actual NPM
references and install them from NPM.
@ellismg ellismg changed the title WIP: Don't yarn link @pulumi dependencies Don't yarn link @pulumi dependencies Apr 30, 2018
lukehoban
lukehoban approved these changes May 1, 2018
View reviewed changes
Copy link
Contributor

@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

aws/aws_test.go
@@ -189,22 +189,22 @@ func Test_Examples(t *testing.T) {
},
{
Dir: path.Join(cwd, "../examples/containers"),
StackName: "containers-fargate",
StackName: addRandomSuffix("containers-fargate"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - any reason we don't do this automatically inside the integration test framework? Seems we'll always need something like this at usage sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do this in the case where you don't provide a StackName explicitly (which is the common case). We explicitly set a StackName here so we can provide a little more context (fargate vs ec2) in the name itself. In this case, we have to do the randomness ourselves.

aws/examples/customDomain/package.json
@@ -6,12 +6,14 @@
"scripts": {
"build": "tsc"
},
"dependencies": {
"@pulumi/pulumi": "^0.12.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be aws?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. Good catch.

pkg/unused.go
@@ -1,5 +0,0 @@
package pkg
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 🔥

CyrusNajmabadi
CyrusNajmabadi approved these changes May 2, 2018
View reviewed changes
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

Seems reasonable given my deeply superficial understanding of how this all works.

@lukehoban lukehoban mentioned this pull request May 4, 2018
Merged
@lukehoban
Copy link
Contributor

@ellismg Anything preventing us from merging this in now?

@ellismg
Copy link
Contributor Author

ellismg commented May 4, 2018

@ellismg Anything preventing us from merging this in now?

Nope.

@ellismg ellismg merged commit 64371ff into master May 4, 2018
@lukehoban lukehoban mentioned this pull request May 9, 2018
Merged
@pgavlin pgavlin deleted the ellismg/no-link branch June 12, 2018 22:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@lukehoban lukehoban lukehoban approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@ellismg @lukehoban @CyrusNajmabadi