-
Notifications
You must be signed in to change notification settings - Fork 880
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
delete recursive=True for ensure_dir #5816
base: main
Are you sure you want to change the base?
Conversation
@ani-sinha , @major , or @xiachen-rh , would any of you be able to review and/or test these changes? I don't have enough background with SELinux to know the full context. |
I don't have much experience with selinix either so can't comment on this. |
Excuse me, may I know if anyone is concerned about this question at the moment? |
Hello, @xiaoge1001 could you provide more context on what this change is doing and more importantly, why? |
Start to check the cause:
|
@TheRealFalcon Sorry I don't have much experience with SELinux either.
As far as l know ifcfg-eth0 is created by cloud-init and not expect to change, am I right?
|
@xiachen-rh I agree with you. I only back up the original ifcfg-eth0 in the system. After the cloud-init is running, run the It doesn't matter whether these steps are reasonable, and I can avoid this problem without modifying the cloud-init code. My question is whether the following operations are correct? if we set recursive to True when path is "/",
|
Hello, is there anyone concerned about this? |
https://github.com/canonical/cloud-init/blob/main/cloudinit/util.py#L1883 path = "/mnt1" "/mnt1" does not exist. What is the meaning of setting recursive=True?
I don't think it's necessary to set recursive=True at this point. |
@xiaoge1001 , I haven't forgotten this issue, but it still seems we have yet to find anybody else with any real expertise here. For something that has been around so long and used by a commonly used utility function, I would really like to find at least one other person with the relevant expertise to review this. Are you on the cloud-init mailing list? It might help asking for help there or in the cloud-init IRC channel on Libera. |
Sorry, I'm not on the mailing list. |
Is that a problem? |
@Conan-Kudo Hey Neal, could you please review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we can't drop this, because ensure_dir()
can be used for a deep hierarchy and not setting the labels from policy for the hierarchy. But this logic also doesn't make sense, because it seems like it assumes there's always only one level up? If that assumption is always true, then yes, we can drop recursive=True
, but I am not sure if it's always used that way.
I don't think it's necessary for “/”. Can we add a judgment condition? If os.path.dirname(path) == "/", set recursive to False. |
Proposed Commit Message
delete recursive=True for ensure_dir
Fixes GH-5807
Additional Context
Test Steps
Merge type