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

Adding Runtime Folder to docker_folders_create function. #7

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

myxptlyk
Copy link
Contributor

@myxptlyk myxptlyk commented Jan 8, 2024

Added runtime folder to scope vars (lines 132,138,144) and to the docker_folders_create function (starting line 572)

@Drauku
Copy link
Contributor

Drauku commented Jan 8, 2024

Lines 579-580 create the .env and compose.yml file inside the ../runtime/stackname/, which I do not think you meant to include.

@Drauku Drauku merged commit d5322d7 into QNAP-HomeLAB:functions Jan 8, 2024
@myxptlyk
Copy link
Contributor Author

myxptlyk commented Jan 8, 2024

Lines 579-580 create the .env and compose.yml file inside the ../runtime/stackname/, which I do not think you meant to include.

Correct. I should have paid more attention to the code I copied and deciphered it better!

Also, just a nitpick, lol. In the scope vars list, "runtime" should be last in the list since r comes after c.

Good call. I didn't consider that it would be better to be alphabetical, but that makes sense. In that case, shouldn't configs and compose be switched too?

@Drauku
Copy link
Contributor

Drauku commented Jan 8, 2024

Good call. I didn't consider that it would be better to be alphabetical, but that makes sense. In that case, shouldn't configs and compose be switched too?

Which is exactly why I removed that comment and declined to make my suggested change. The current grouping with "paths" at the top and the compose file variable at the bottom is more intuitive, IMHO.

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.

2 participants