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

Added clean_parent argument for the archive state #55418

Merged
merged 4 commits into from Dec 3, 2019

Conversation

Oloremo
Copy link
Contributor

@Oloremo Oloremo commented Nov 23, 2019

What does this PR do?

Adding new argument clean_parent to the archive state. This allows the state to remove parent directory before the extraction.

What issues does this PR fix or reference?

#54012

Previous Behavior

archive state was unable to do so by itself.

New Behavior

archive state now able to remove parent directory if requested

Tests written?

Yes... kinda.

Commits signed with GPG?

Yes

@Oloremo Oloremo requested a review from as a code owner Nov 23, 2019
@ghost ghost requested a review from cassandrafaris Nov 23, 2019
@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Nov 23, 2019

The only thing that I test is the conflict check between clean and clean_parent.
The archive.extracted state is huge and only have 3 test cases, right now I don't really have an idea how to test this new logic. Any advice welcome.

@Oloremo Oloremo force-pushed the 54012-clean-parent-archive branch 2 times, most recently from 8931c63 to 6ab9c01 Compare Nov 23, 2019
Copy link
Collaborator

@terminalmage terminalmage left a comment

Just a few minor changes.

@@ -522,6 +523,12 @@ def extracted(name,
.. versionadded:: 2016.11.1
clean_parent : False
Set this to ``True`` to remove a parent archive directory before the extration.
Copy link
Collaborator

@terminalmage terminalmage Nov 25, 2019

Choose a reason for hiding this comment

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

I would reword this like so:

        If ``True``, and the archive is extracted, delete the parent
        directory (i.e. the directory into which the archive is extracted), and
        then re-create that directory before extracting. Note that ``clean``
        and ``clean_parent`` are mutually exclusive.

Copy link
Contributor Author

@Oloremo Oloremo Nov 25, 2019

Choose a reason for hiding this comment

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

thanks, done!

@@ -1076,6 +1083,11 @@ def extracted(name,
))
return ret

if clean and clean_parent:
ret['comment'] = "You can't set both 'clean' and 'clean_parent' to True."
Copy link
Collaborator

@terminalmage terminalmage Nov 25, 2019

Choose a reason for hiding this comment

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

We don't typically use the phrasing "you can't". Instead, can you change this to Only one of 'clean' and 'clean_parent' can be set to True?

Copy link
Contributor Author

@Oloremo Oloremo Nov 25, 2019

Choose a reason for hiding this comment

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

thanks, done!

try:
salt.utils.files.rm_rf(name.rstrip(os.sep))
ret['changes'].setdefault(
'removed', "Directory {} was removed prior to the extraction".format(name))
Copy link
Collaborator

@terminalmage terminalmage Nov 25, 2019

Choose a reason for hiding this comment

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

Since you've removed the directory, you need to recreate it now. And you also need to take into account the value of the makedirs argument when you do. Otherwise, the destination directory won't exist when it comes time to extract, and the extraction will fail.

Copy link
Contributor Author

@Oloremo Oloremo Nov 25, 2019

Choose a reason for hiding this comment

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

Correct me if I'm wrong but parent directory going to be created no matter what:

1286         if not os.path.isdir(name):
1287             __states__['file.directory'](name, user=user, makedirs=True)
1288             created_destdir = True

Copy link
Collaborator

@terminalmage terminalmage Nov 25, 2019

Choose a reason for hiding this comment

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

Ahh yes, you are correct. I mistakenly thought that this state used a makedirs param, I must have been confusing it with something else.

@Oloremo Oloremo force-pushed the 54012-clean-parent-archive branch from 027b3c6 to 4692664 Compare Nov 25, 2019
@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Nov 25, 2019

Did a rebase + added a changelog

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Nov 26, 2019

re-run full centos7-py2-m2crypto

@Oloremo
Copy link
Contributor Author

@Oloremo Oloremo commented Nov 26, 2019

re-run full amazon2-py2

Akm0d
Akm0d approved these changes Nov 26, 2019
@Akm0d Akm0d added this to the Approved milestone Nov 26, 2019
@Akm0d Akm0d added this to In Progress in Neon Nov 26, 2019
@Akm0d Akm0d self-assigned this Nov 26, 2019
ret['comment'] += (
' Since the \'clean_parent\' option is enabled, the '
'destination parent directory would be removed first '
'and than re-created and the archive would be '
Copy link
Contributor

@waynew waynew Dec 2, 2019

Choose a reason for hiding this comment

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

Typo:

Suggested change
'and than re-created and the archive would be '
'and then re-created and the archive would be '

Could also be re-written to include the actual path that will be purged (using .format). But definitely fix the typo.

Copy link
Contributor Author

@Oloremo Oloremo Dec 2, 2019

Choose a reason for hiding this comment

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

thanks, fixed

Copy link
Contributor

@waynew waynew left a comment

Small typo

@Oloremo Oloremo force-pushed the 54012-clean-parent-archive branch from d86cc9e to 9047b01 Compare Dec 2, 2019
@dwoz dwoz merged commit 05482c7 into saltstack:master Dec 3, 2019
49 checks passed
@sagetherage sagetherage moved this from In Progress to Done in Neon Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Neon
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants