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.

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

Allow loading of sparkle packs #216

Merged
merged 7 commits into from
Mar 1, 2018
Merged

Allow loading of sparkle packs #216

merged 7 commits into from
Mar 1, 2018

Conversation

patrobinson
Copy link
Contributor

SparkleFormation supports the concept of a SparklePack which can be packaged as a RubyGem and required inside the template that utilises it. This allows re-usable dynamics to be shared very simply.

Unfortunately the implementation is fundamentally broken. Such that requiring a sparkle pack inside a template like so (which is what the documentation suggests):

require 'vpc_sparkle_pack'
pack = ::SparkleFormation::SparklePack.new(:name => 'vpc_sparkle_pack')
SparkleFormation.new(:util_vpc, sparkle: pack) do
...
end

results in a race condition, dependent on the file load order which is not deterministic.

This bug occurs when SparkleFormation tries to load a SparklePack while it's still loading templates/. Therefore the easiest workaround is to pre-load the SparklePack from StackMaster.

I would've liked to have fixed this in SparkleFormation, but after spending 3 days on the problem I feel I am no closer to a solution.

README.md Outdated
my-stack:
template: my-stack-with-dynamic.rb
compiler_options:
sparkle_pack:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be sparkle_packs

Copy link
Contributor Author

@patrobinson patrobinson Feb 28, 2018

Choose a reason for hiding this comment

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

Fixed in b593862

@@ -467,6 +467,20 @@ stacks:
template: my-stack.rb
```

### Loading SparklePacks

[SparklePacks](http://www.sparkleformation.io/docs/sparkle_formation/sparkle-packs.html) can be pre-loaded using compiler options. This requires the name of a rubygem to `require` followed by the name of the SparklePack, which is usually the same name as the Gem.
Copy link
Contributor

@gstib gstib Feb 28, 2018

Choose a reason for hiding this comment

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

Might be worth stating that we are deviating from SparkleFormation prescribed technique.
i.e. You wont be doing this

require 'my_sparkle_pack'

custom_pack = SparkleFormation::SparklePack.new(:name => 'my_sparkle_pack')
sfn = SparkleFormation.new(:my_template) do
   dynamic!(:my_dynamic)
end
sfn.sparkle.add_sparkle(custom_pack)

It's just

SparkleFormation.new(:my_template) do
   dynamic!(:my_dynamic)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 978dfea

README.md Outdated
template: my-stack-with-dynamic.rb
compiler_options:
sparkle_pack:
vpc-sparkle-pack: vpc-sparkle-pack
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is a little confusing as well. I can see it's gemname: packname but aren't they the same thing...
Can you get rid of the packname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can be different, when you call register! it's possible to supply a different name.

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is what value would it add to be different. Why would someone want/need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, but SparkleFormation supports it so I'm inclined to maintain that interface unless there's some compelling reason not to.

Copy link
Contributor

Choose a reason for hiding this comment

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

When you traditionally loaded like this

require 'my_sparkle_pack'

custom_pack = SparkleFormation::SparklePack.new(:name => 'my_sparkle_pack')
sfn = SparkleFormation.new(:my_template) do
   dynamic!(:my_dynamic)
end
sfn.sparkle.add_sparkle(custom_pack)

They actually needed to be the same to work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand it's entirely possible to load it like this:

require 'my_sparkle_pack'

custom_pack = SparkleFormation::SparklePack.new(:name => 'wow_dude_a_sparkle_pack')

Where my_sparkle_pack registers itself like so:

SparkleFormation::SparklePack.register!('wow_dude_a_sparkle_pack')

Copy link
Contributor

@gstib gstib Feb 28, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a supported API but whatever, done in 00e8993

Patrick Robinson added 2 commits February 28, 2018 15:20
and how it will behave in cases of duplicate dynamics
README.md Outdated
template: my-stack-with-dynamic.rb
compiler_options:
sparkle_packs:
vpc-sparkle-pack: vpc-sparkle-pack
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to change sparkle_packs to a list of sparkle packs to preserve order. I'm not sure if this structure will guarantee it. Ordering looks important if you want to override things. See here

Copy link
Contributor

@simpsora simpsora left a comment

Choose a reason for hiding this comment

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

LGTM (I think)

Copy link
Contributor

@shivaman shivaman left a comment

Choose a reason for hiding this comment

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

If Sparkleformation fixes it at some time in the future, will we revert this?

I think it is a safe to assume if Sparkleformation changes the behaviour, it will be a major version change, and we will need to revisit stack_master anyway 🤷‍♂️

@patrobinson
Copy link
Contributor Author

If Sparkleformation fixes it at some time in the future, will we revert this?

Yes, I think it's best to follow the SparkleFormation documented golden path where possible

@patrobinson patrobinson merged commit a83f703 into master Mar 1, 2018
@patrobinson patrobinson deleted the load-sparkle-packs branch March 1, 2018 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants