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

Add extension ordering #242

Merged
merged 14 commits into from
Oct 3, 2023
Merged

Add extension ordering #242

merged 14 commits into from
Oct 3, 2023

Conversation

agyoungs
Copy link
Contributor

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.

Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

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.

src/rocker/core.py Outdated Show resolved Hide resolved
src/rocker/core.py Outdated Show resolved Hide resolved
src/rocker/core.py Outdated Show resolved Hide resolved
src/rocker/core.py Outdated Show resolved Hide resolved
@agyoungs
Copy link
Contributor Author

agyoungs commented Sep 22, 2023

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
Copy link
Collaborator

tfoote commented Sep 22, 2023

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.

@agyoungs
Copy link
Contributor Author

agyoungs commented Sep 23, 2023

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.

Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this with me and including test coverage

@tfoote tfoote merged commit 3afa6ac into osrf:main Oct 3, 2023
2 checks passed
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.

Add the ability for extensions to have requirements on other extension
2 participants