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

refactor: replace distutils' copy_tree with shutil copytree #964

Merged
merged 3 commits into from
Feb 4, 2024

Conversation

gruzewski
Copy link
Contributor

@gruzewski gruzewski commented Mar 10, 2023

Fixes issue #963 (partially)

Proposed Changes

distutils.dir_util module is deprecated and will be removed from 3.12 (see this). Unfortunately, shutil.copytree is not a drop-in replacement as it returns destination directory and not the list of copied files, like in distutils.dir_util.copy_tree.

This PR adds a simple wrapper around copy_tree that computes copied files.

Testing

  • Added basic unit tests
  • Manually ran kapitan init --directory and got the same results. Cases covered: empty dir, existing dir without the structure, existing dir with the structure in place

NOTE: This change only works with Python >= 3.8 as dirs_exist_ok was added there. Otherwise, it will throw FileExistsError.

@gruzewski gruzewski force-pushed the remove_distutil_copy_tree branch 3 times, most recently from 118f8ae to 671cdcd Compare March 27, 2023 20:48
@MatteoVoges
Copy link
Contributor

Hey @gruzewski ,
Thanks for noticing that the package is depricated or will be depricated soon. But instead of using a wrapper, wouldn't it be better to adapt our functionality and use the new function from the shutil library? There should be a way that we replace the list with the copied files which some os function like listdirs()

@gruzewski
Copy link
Contributor Author

@MatteoVoges I did some research and couldn't develop a better solution. The challenge with os.listdir() is that it doesn't support recursive listing. And if someone initialises an existing directory (like an infrastructure repo with a .git folder and maybe README.md) with Kapitan inventory, we would need a different way to only list new files (like using a timestamp, but that feels very fragile). Otherwise, we would go back to this issue #306.

@MatteoVoges
Copy link
Contributor

@gruzewski What if we traverse the template dir? Then we would get only the files, which get populated/ copied...
I think with your pr #966 and the tree output we don't even need information about which files existed already and which don't. We only need the current state of the directory

@github-actions github-actions bot added the Stale label Nov 10, 2023
@sapslaj
Copy link

sapslaj commented Jan 25, 2024

Now that Python 3.12 is out, Kapitan can't be run and throws a ModuleNotFoundError: No module named 'distutils'. PEP 632's migration advice even suggests reimplementing distutils.dir_util.create_tree.

@MatteoVoges what do you think about merging this as-is to enable Python 3.12 support? I realize it's been quite a while so if @gruzewski isn't interested in fixing up the branch I'd be willing to take that on.

@gruzewski
Copy link
Contributor Author

I can pick it up next week :)

@gruzewski
Copy link
Contributor Author

I have rebased the PR, and the tests are passing, so this is good to go.

@MatteoVoges, coming back to your point - we could technically do that, but there is a risk of reporting files that failed to copy. It wouldn't be a good first impression for users.

@MatteoVoges
Copy link
Contributor

I have rebased the PR, and the tests are passing, so this is good to go.

Thanks, I will have a look at it soon!

@MatteoVoges, coming back to your point - we could technically do that, but there is a risk of reporting files that failed to copy. It wouldn't be a good first impression for users.

I don't see it that critical if we just dump the tree output. Can you explain once again why we need the different handling for new vs. old files?

@MatteoVoges MatteoVoges mentioned this pull request Feb 2, 2024
2 tasks
Copy link
Contributor

@MatteoVoges MatteoVoges left a comment

Choose a reason for hiding this comment

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

Well done!

@github-actions github-actions bot removed the Stale label Feb 3, 2024
@ademariag
Copy link
Contributor

Thank you @MatteoVoges and @gruzewski

@ademariag ademariag merged commit be31d36 into kapicorp:master Feb 4, 2024
7 checks passed
@gruzewski gruzewski deleted the remove_distutil_copy_tree branch February 4, 2024 21:31
@MatteoVoges
Copy link
Contributor

@gruzewski Could you remove distutils from pyproject.toml and update the lockfile?

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.

4 participants