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

Ensure prepare_container works as expected in App #1185

Merged
merged 1 commit into from Jul 17, 2022

Conversation

timriley
Copy link
Member

We want to ensure that the prepare_container block is run at the last moment before the container is marked as configured!. With this change, this is now true for both App and Slice.

Before this change, the prepare_container block for App was being run before the app-specific configuration was applied.

@@ -219,6 +219,7 @@ def prepare_slice

prepare_all

instance_exec(container, &@prepare_container_block) if @prepare_container_block
Copy link
Member Author

Choose a reason for hiding this comment

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

This is basically the change, moving this instance_exec to a different spot. Everything else in this PR is ensuring we now have appropriate test coverage for this behaviour :)

@timriley timriley force-pushed the consistent-prepare-container-in-app-and-slice branch 2 times, most recently from 75da435 to ad6c18f Compare July 17, 2022 09:36
We want to ensure that the `prepare_container` block is run at the _last moment_ before the container is marked as `configured!`. With this change, this is now true for both `App` and `Slice`.

Before this change, the `prepare_container` block for `App` was being run before the app-specific configuration was applied.
@timriley timriley force-pushed the consistent-prepare-container-in-app-and-slice branch from ad6c18f to 7defc12 Compare July 17, 2022 09:37
@timriley timriley self-assigned this Jul 17, 2022
@timriley timriley added this to the v2.0.0 milestone Jul 17, 2022
@timriley
Copy link
Member Author

@timriley
Copy link
Member Author

@jodosha this is mostly a PR to ensure completeness/consistency in the existing prepare_container behaviour, which until #1162 existed on Slice only. Now that it's on App and Slice, it needed a little tweak. As such, I'm going to merge it in now, but happy to discuss it post-merge if need be 😄

@timriley timriley merged commit 1705bb1 into main Jul 17, 2022
@timriley timriley deleted the consistent-prepare-container-in-app-and-slice branch July 17, 2022 09:59
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.

None yet

1 participant