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

[WIP] Added clean_top argument for the archive state #54013

Closed

Conversation

Oloremo
Copy link
Contributor

@Oloremo Oloremo commented Jul 25, 2019

What does this PR do?

Adding new argument clean_top to the archive state. This allows the state to remove top level 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 top level directory if requested

Tests written?

No(yet) - RFC required first

Commits signed with GPG?

Yes

@Oloremo Oloremo requested a review from a team as a code owner July 25, 2019 17:32
@Oloremo
Copy link
Contributor Author

Oloremo commented Jul 25, 2019

Folks, could you please look at the mentioned issue and in my implementation of this feature and tell if it's ok(both issue and implementation). If it's ok for you I'll write tests(not yet sure how)

@Oloremo Oloremo force-pushed the added_clean_top_for_archive branch from 831d955 to b63d87b Compare July 25, 2019 17:38
@ghost ghost requested a review from Akm0d July 25, 2019 18:01
@@ -526,6 +527,9 @@ def extracted(name,

.. versionadded:: 2016.11.1

clean_top : False
Set this to ``True`` to remove a top level directory before the extration.
Copy link
Contributor

Choose a reason for hiding this comment

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

extraction

errors = []
log.debug('Removing directory %s due to clean_top set to True', name)
try:
salt.utils.files.rm_rf(name.rstrip(os.sep))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename the directory, if the archive.extract is successful, then delete, otherwise roll it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure here.
First of all, it adds complexity + a hidden requirement for a 2x space(to keep both old and new dir at some point of time).
Second is that the current clean: True logic doesn't do it and I feel like it's more or less the same case.

@Oloremo
Copy link
Contributor Author

Oloremo commented Nov 23, 2019

Closed in favor of #55418

@Oloremo Oloremo closed this Nov 23, 2019
@Oloremo Oloremo deleted the added_clean_top_for_archive branch November 23, 2019 22:33
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.

2 participants