osrf / rocker

A tool to run docker containers with overlays and convenient options for things like GUIs etc.
Apache License 2.0
567 stars 74 forks source link

Add extension ordering #242

Closed agyoungs closed 1 year ago

agyoungs commented 1 year ago

Closes #37

Adds the ability to specify required and preceding extensions for any given extension. The 'user' extension is a special case that will always be loaded last unless an extension specifically requests the 'user' extension to be loaded before.

I pulled together snippets from the groot_rocker repo created by @stonier and made a few modifications of my own.

agyoungs commented 1 year ago

Thanks for putting this together. I like the overall approach. I have a few requests with respect to the logic, and think that updating the behavior with respect to USER as in #243 is a cleaner approach to special casing USER. Where the extension still does things but setting USER is special.

Are you suggesting changing the special casing of user in the PR I've made? I see that in #243 you're creating a pre/post USER dockerfile snippet. In that case I think you may still want to ensure the user extension is present and loaded before applying the extension in question.

tfoote commented 1 year ago

Are you suggesting changing the special casing of user in the PR I've made? I see that in https://github.com/osrf/rocker/pull/243 you're creating a pre/post USER dockerfile snippet. In that case I think you may still want to ensure the user extension is present and loaded before applying the extension in question.

Right it may be that your plugin wants to go before or after the user plugin snippet. But you can declare that for your plugin. What I've done in #243 is separate the User plugin snippet from the USER keyword switching. So if you want the user to be created on the filesystem before you do something but still as root you use a root_snippet and declare following Execute after user. If you want something to execute after the user id has been switched via the USER command you use the user_snippet command. This gives you the flexibility to go before or after the user snippet and before or after the USER switch without requiring any special case sorting of the User plugin.

agyoungs commented 1 year ago

Are you suggesting changing the special casing of user in the PR I've made? I see that in #243 you're creating a pre/post USER dockerfile snippet. In that case I think you may still want to ensure the user extension is present and loaded before applying the extension in question.

Right it may be that your plugin wants to go before or after the user plugin snippet. But you can declare that for your plugin. What I've done in #243 is separate the User plugin snippet from the USER keyword switching. So if you want the user to be created on the filesystem before you do something but still as root you use a root_snippet and declare following Execute after user. If you want something to execute after the user id has been switched via the USER command you use the user_snippet command. This gives you the flexibility to go before or after the user snippet and before or after the USER switch without requiring any special case sorting of the User plugin.

I'm on the same page now. I think the approach is cleaner in #243 and gives more flexibility. It's up to the extension creator to determine if the user option is required. If the user extension is not enabled the user snippet will run as ROOT (which is fine and expected).

I think I've made all the changes you've requested, but feel free to raise an issue if you see one. We might want to consider updating any of the extensions included in this repo with the required method if any extensions do require others to function properly.