Refactor output lookup #550
Conversation
stacker/context.py
Outdated
| self._stacks.append(stack) | ||
| return self._stacks | ||
|
|
||
| def get_stack(self, name): |
There was a problem hiding this comment.
Needs docs.
Also, if you're wondering why this doesn't use get_stacks_dict, that function indexes by stack.fqn, which we don't want. We want to index by the name of the node in the graph.
| # from. | ||
| handler = partial(output_handler, rfqn=True) | ||
|
|
||
| def handler(value, provider=None, context=None, **kwargs): |
There was a problem hiding this comment.
Since the xref and rxref lookups can't re-use the output lookup, I just went ahead and re-implemented them to be more explicit, since they were pretty hard to follow before with all of the function currying.
| raise ValueError('Context is required') | ||
|
|
||
| d = deconstruct(value) | < 8000 /tr>||
| stack_fqn = context.get_fqn(d.stack_name) |
There was a problem hiding this comment.
The original rxref implementation does the following, instead of using context.get_fqn:
I'm not sure I understand the original reasoning for that, since it looks identical to what context.get_fqn does, and tests continue to pass. If there is a reason, we need better tests.
|
|
||
| def __repr__(self): | ||
| return self.fqn | ||
|
|
||
| @property | ||
| def requires(self): | ||
| requires = set([self.context.get_fqn(r) for r in | ||
| self.definition.requires or []]) | ||
| requires = set(self.definition.requires or []) |
There was a problem hiding this comment.
Technically this is a breaking change, but one that I doubt would effect very many people. requires would no longer support specifying an fqn, instead it would only support using the node name in the graph (same as how the output lookup works).
There was a problem hiding this comment.
Can you run this by the folks in the stacker channel, just to make sure we aren't breaking anything for anyone we know of?
|
|
||
| output = provider.get_output(stack_fqn, d.output_name) | ||
| return output | ||
| stack = context.get_stack(d.stack_name) |
There was a problem hiding this comment.
Because I figure this question will get asked, "What happens if get_stack returns None, or outputs haven't been set?":
The graph ensures that that scenario can't happen (remember, the graph is being built out based on the output lookups). If for some reason it did happen, then the step would just blow up, and any downstream steps would be cancelled.
Codecov Report
@@ Coverage Diff @@
## master #550 +/- ##
=========================================
- Coverage 87.55% 87.46% -0.1%
=========================================
Files 95 95
Lines 6108 6126 +18
=========================================
+ Hits 5348 5358 +10
- Misses 760 768 +8
Continue to review full report at Codecov.
|
35500f6 to
ad6979d
Compare
stacker/context.py
Outdated
| if stack.name == name: | ||
| return stack | ||
|
|
||
| def get_stacks_dict(self, fqn=True): |
There was a problem hiding this comment.
It doesn't look like the fqn argument is used?
| def set_outputs(self, stack_name, stack): | ||
| self._outputs[stack_name] = get_output_dict(stack) | ||
| return | ||
| def get_output_dict(self, stack): |
There was a problem hiding this comment.
Yeah, this gets used in the actions to strip outputs from provider_stack. I'd rather providers just return something like a stacker.providers.base.Stack object instead, but that's a bigger refactor.
|
|
||
| def __repr__(self): | ||
| return self.fqn | ||
|
|
||
| @property | ||
| def requires(self): | ||
| requires = set([self.context.get_fqn(r) for r in | ||
| self.definition.requires or []]) | ||
| requires = set(self.definition.requires or []) |
There was a problem hiding this comment.
Can you run this by the folks in the stacker channel, just to make sure we aren't breaking anything for anyone we know of?
I ran it poll and nobody said they were using this. Makes sense; just using |
|
@ejholmes re:
FWIW, we mostly use requires + native export/import over the output/param pattern because of the strong guarantees we get from pushing the dependency information into the CFN domain. |
Cool, good to know. We don't have any plans to get rid of |
Refactor output lookup
This refactors the mechanics of the
${output}lookup so that it uses the internalstacker.Stackas the lookup point.With the addition of the DAG, in order to ensure that output lookups didn't involve unnecessary network calls, we changed
stacker.actions.buildto callprovider.set_outputsto store the stacks outputs when the step was completed.https://github.com/remind101/stacker/blob/c1aad4f8713c64c90abc02e6c10ad2eda1a18c8e/stacker/actions/build.py#L262-L264
The output lookup would then use this cached value, through
provider.get_output.https://github.com/remind101/stacker/blob/c1aad4f8713c64c90abc02e6c10ad2eda1a18c8e/stacker/lookups/handlers/output.py#L44-L49
This was the smallest change that we could make in the DAG branch, while keeping everything fast and functional, but won't work well moving forward as we think about cross account/region support, as well as the possibility of adding new node types to the graph (e.g. hooks).
In this PR, when a stack is "done", we store the outputs on the internal
stacker.Stackobject. Theoutputplugin looks up the stack object, then returns the output from there. This has a couple of advantages:outputlookup needs to be aware that these nodes in the graph are separate things.