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

Implement hardlink() for both salt.modules.file and salt.states.file #55000

Merged
merged 13 commits into from Dec 6, 2019

Conversation

arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Oct 14, 2019

What does this PR do?

This adds the hardlink() function as intended by PR #20428 but was dropped by the author. PR #20428 seemed to be a copy-paste job from symlink(), and although this PR is based on it. The semantics in this PR were changed so that it made sense for hard links. (i.e. you can't hard link a directory, lack of windows support, etc.) Hard link symmetry is determined by comparing the inode of the source and target paths.

What issues does this PR fix or reference?

This closes issue #39475 which was actually closed originally due to the issue going stale.

Previous Behavior

There was no way to create hard links on posixy environments.

New Behavior

There are now two functions in salt.modules.file. These are the .is_hardlink() which checks the st_nlink field to determine if something is a hard link, and .hardlink() which can be used to actually make the link. There is also a new state in salt.states.file which allows one to create a hard link without overwriting a non-hardlink unless the force option is provided.

Tests written?

Yes. It was very painful. It is 4am over here.

Commits signed with GPG?

No.

…s "link()". Culled the new one, and updated everything to use the old one.
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 16, 2019

(Force-push'd to cull out reverts)

…dlink()" function instead of the correct "link()".
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Oct 16, 2019

Okay. I really need help on this. Somebody halp plz.

In this PR, I haven't touched any of the following lines. I do use CommandExecutionError, but that shouldn't matter between Python2/3. The issue is that empty space between the imports and the if-test results in a lint error of incompatible-py3-code. I developed this and the testcases using Python3.7, and all of the Python3 tests passed, so this doesn't appear to be correct...

These are the relevant lines (according to the lint check) in salt.states.file at line +314. Again, line +314 is the white space in the center.

import salt.utils.versions
from salt.exceptions import CommandExecutionError
from salt.serializers import DeserializationError
from salt.state import get_accumulator_dir as _get_accumulator_dir

if salt.utils.platform.is_windows():
    import salt.utils.win_dacl
    import salt.utils.win_functions
    import salt.utils.winapi

@dwoz dwoz requested a review from as a code owner Dec 3, 2019
@ghost ghost requested a review from Akm0d Dec 3, 2019
Copy link
Contributor

@dwoz dwoz left a comment

@arizvisa The linter is un-happy

@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 4, 2019

@arizvisa The linter is un-happy

Yeah, my previous message mentions the following, and I'm not sure how to proceed here because I haven't touched the relevant lines at all, and the PR works in Python3 for me.

Okay. I really need help on this. Somebody halp plz.

In this PR, I haven't touched any of the following lines. I do use CommandExecutionError, but that shouldn't matter between Python2/3. The issue is that empty space between the imports and the if-test results in a lint error of incompatible-py3-code. I developed this and the testcases using Python3.7, and all of the Python3 tests passed, so this doesn't appear to be correct...

These are the relevant lines (according to the lint check) in salt.states.file at line +314. Again, line +314 is the white space in the center.

import salt.utils.versions
from salt.exceptions import CommandExecutionError
from salt.serializers import DeserializationError
from salt.state import get_accumulator_dir as _get_accumulator_dir

if salt.utils.platform.is_windows():
    import salt.utils.win_dacl
    import salt.utils.win_functions
    import salt.utils.winapi

…e it's fucking stupid and can't distinguish when the builtin is actually being properly iterated through.
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 4, 2019

@dwoz, apparently from debugging PyLint it believes that
for direction, item in zip(['to', 'from'], [name, target]): is not being iterated through and is hence failing pylint.checkers.python3._in_iterating_context.

This is because pylint's checker is looking for For to distinguish this via the following code in _in_iterating_context.

    if isinstance(parent, astroid.For):
        return True

However, the parent for zip there is actually

>>> list(b[1].get_children())[0]
<Name.zip l.1 at 0x7f522f3b55d0>
>>> list(b[1].get_children())[0].parent
<Call l.1 at 0x7f522f3b5590>

which instead takes this branch and *_ACCEPTS_ITERATOR' doesn't include zip`:

    elif isinstance(parent, astroid.Call):
        if isinstance(parent.func, astroid.Name):
            if parent.func.name in _ACCEPTS_ITERATOR:
                return True
        elif isinstance(parent.func, astroid.Attribute):
            if parent.func.attrname in ATTRIBUTES_ACCEPTS_ITERATOR:
                return True

Instead of sitting around and waiting for PyLint to fix their issue which would then block this PR,
I replaced my usage of zip with zip_longest (not referenced at all in PyLint) so that PyLint doesn't even bother to try and execute this busted code and will thus not even try to determine whether it's being iterated through or not.

   if node.func.name in ("filter", "map", "range", "zip"):
                    if not _in_iterating_context(node):
                        checker = "{}-builtin-not-iterating".format(node.func.name)
                        self.add_message(checker, node=node)

All our tools are fucked..

@dwoz
Copy link
Contributor

@dwoz dwoz commented Dec 6, 2019

@arizvisa In the future you can also skip that check for the line pylint doesn't understand # pylint: check-name-or-id

dwoz
dwoz approved these changes Dec 6, 2019
@dwoz dwoz merged commit 1404eaa into saltstack:master Dec 6, 2019
49 checks passed
@arizvisa
Copy link
Contributor Author

@arizvisa arizvisa commented Dec 7, 2019

@dwoz, gotcha. thanks for the heads up.

arizvisa added a commit to arizvisa/lolfuzz3 that referenced this issue May 24, 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

2 participants