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 check that exporter path exists #1024

Merged
merged 1 commit into from Nov 25, 2020
Merged

Conversation

daviddavis
Copy link
Contributor

@daviddavis daviddavis commented Nov 12, 2020

Adding check that exporter path exists before trying to create and modify it.

fixes #7829

Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html

@pulpbot
Copy link
Member

pulpbot commented Nov 12, 2020

Attached issue: https://pulp.plan.io/issues/7829

Comment on lines -171 to +173
os.makedirs(pulp_exporter.path, exist_ok=True)
os.chmod(pulp_exporter.path, 0o775) # let owner and group read and write the directory
path = Path(pulp_exporter.path)
if not path.is_dir():
path.mkdir(mode=0o775, parents=True)
Copy link
Member

Choose a reason for hiding this comment

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

The only difference i see in this change is that the chmod part is only applied, when the directory does not already exist.
(And maybe closing a race condition window by setting the mode while creating).
That is not exactly matching how i read the changelog.
Also, how about first making sure the directories exist, and then executing the chmod only if we don't have the necessary permissions?
Paraphrasing:

        path = Path(pulp_exporter.path)
        if not path.is_dir():
            path.mkdir(mode=0o775, parents=True)
        elif cannot_write:
            os.chmod(path, 0o775)

If the chmod fails, it shall not be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I am solving is that the directory exists and is writable but the worker doesn’t own it and thus can’t chmod it. Agreed that the change log and commit message need to be updated to convey that.

If the directory exists though and is not writable, I think it should just fail and not try to chmod it. But I can be convinced otherwise. The initial problem this chmod tried to solve was that pulp was creating directories that weren’t writable.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are probably right that we should not touch permissions on directories already existing.

@daviddavis daviddavis merged commit 9b0c86e into pulp:master Nov 25, 2020
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

4 participants