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

Remove dependency on cookiecutter #3526

Closed
wants to merge 3 commits into from
Closed

Remove dependency on cookiecutter #3526

wants to merge 3 commits into from

Conversation

zhan9san
Copy link
Contributor

@zhan9san zhan9san commented Apr 25, 2022

Fix #3486

@ssbarnea

Both cookiecutter and ansible template are using Jinja. So I decide to continue to use Jinja as a template engine.

To avoid reinventing the wheel, the template render logic is migrated from cookiecutter with tailored function and parameters.

I'd like to confirm with you whether the above way is accepted.
If yes, I'll add more tests later and fix the lint issue in this PR.
If no, do you have any suggestions?

To do

Do you have any concerns for to-do?

  1. Fix CI issue in this PR
  2. Add template directory with out {{ and mark {{ directory deprecated in another PR

Backward compatible

Currently, the following commands work as before. It means, this change is backward-compatible, and for other drivers, too.

  1. delegated by default
$ molecule init role acme.my_new_role_docker --driver-name docker
INFO     Initializing new role my_new_role_docker...
No config file found; using defaults
- Role my_new_role_docker was created successfully
[WARNING]: No inventory was parsed, only implicit localhost is available
localhost | CHANGED => {"backup": "","changed": true,"msg": "line added"}
INFO     Initialized role in /Users/jackzhang/tmp/my_new_role_docker successfully.
  1. docker driver
molecule init role acme.my_new_role
INFO     Initializing new role my_new_role...
No config file found; using defaults
- Role my_new_role was created successfully
[WARNING]: No inventory was parsed, only implicit localhost is available
localhost | CHANGED => {"backup": "","changed": true,"msg": "line added"}
INFO     Initialized role in /Users/jackzhang/Downloads/tmp/roles/my_new_role successfully.

Related: cookiecutter/cookiecutter#1555 cookiecutter/cookiecutter#1636 cookiecutter/cookiecutter#1642

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

That looks awesome but I did not perform a deep review as I am still in vacation. Lets get help from at least one more reviewer before acting as we do want to be confident in it.

@ssbarnea ssbarnea changed the title drop cookiecutter package Remove dependency on cookiecutter Apr 26, 2022
@zhan9san
Copy link
Contributor Author

@ssbarnea

Enjoy yourself.

At least, you made me think I am on the right track.

@ssbarnea ssbarnea requested a review from apatard April 28, 2022 07:56
@ssbarnea
Copy link
Member

@zhan9san Do not forget to fix the linting.

@zhan9san
Copy link
Contributor Author

@ssbarnea

I am working on it.

Copy link
Contributor

@apatard apatard left a comment

Choose a reason for hiding this comment

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

I'm seeing no documentation change so I assume that it fully transparent to the user ?


# Add the Python object to the context dictionary
file_name = os.path.split(context_file)[1]
file_stem = file_name.split(".")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

why not using something like (untested) ? :

file_name = os.path.basename(context_file)
file_stem = file_name.splitext(file_name)[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @apatard

Thanks for your review.

It is referred to cookiecutter generater.py

They do the same thing getting the filename without extension.

Copy link
Contributor Author

@zhan9san zhan9san May 6, 2022

Choose a reason for hiding this comment

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

Hi @apatard

file_name generated by os.path.basename(context_file) is string, which does not have splitext function.

So I only change the file_name from os.path.split(context_file)[1] to os.path.basename(context_file).

Choose a reason for hiding this comment

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

I think @apatard meant os.path.splitext(file_name)[0] and not file_name.splitext(file_name)[0]. While I think that would be better, it is a potentially breaking change, since the current code splits at the first ., while os.path.splitext splits at the last ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @felixfontein

Thanks for your review. You are right.

The file_stem is generated by os.path.splitext(file_name)[0] in new commit.

For this specific case, as cookiecutter.json is used, the value of file_stem would be cookiecutter.

@zhan9san
Copy link
Contributor Author

zhan9san commented May 6, 2022

@ssbarnea

All CI, including lint jobs pass.

Would you kindly review this PR?

@ssbarnea ssbarnea requested a review from apatard May 6, 2022 16:56
@@ -0,0 +1,61 @@
"""cookiecutter main function."""

Choose a reason for hiding this comment

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

I'm not a lawyer, but if this contains code that was taken from cookiecutter, doesn't it need to carry the license and copyright from cookiecutter? (Same for other code taken from that project.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfontein

Thanks for your reminder.

The cookiecutter license and copyright are added in new commit, referring to how interpolation.py did.
https://github.com/ansible-community/molecule/blob/04157c3684a46987ba00a6223261e3da30ff5b0c/src/molecule/interpolation.py#L1-L16

@ssbarnea ssbarnea requested a review from cidrblock May 6, 2022 19:34
@zhan9san
Copy link
Contributor Author

zhan9san commented May 7, 2022

I'm seeing no documentation change so I assume that it fully transparent to the user ?

Yes, it's a backward compatible migration.

@ssbarnea ssbarnea requested a review from felixfontein May 9, 2022 11:09
@ssbarnea
Copy link
Member

ssbarnea commented May 9, 2022

@apatard @felixfontein Any more concerns?

@zhan9san
Copy link
Contributor Author

zhan9san commented Jun 1, 2022

Cookiecutter is live again - we released 2.1.0 https://pypi.org/project/cookiecutter/2.1.0/

Originally posted by @jensens in #3486 (comment)

If we continue to use cookiecutter, we can close this PR.

@ssbarnea
Copy link
Member

ssbarnea commented Jun 1, 2022

Closed in favour of #3568 -- please rebase that one. I want to merge that change in a feature branch as cookiecutter had finnally got a new release and we might not need to remove it. Still, i do not want to lose your work.

@ssbarnea ssbarnea closed this Jun 1, 2022
@zhan9san
Copy link
Contributor Author

zhan9san commented Jun 1, 2022

Thanks.

I think it's not urgent. I'll try to make it work.

Besides, if cookiecutter works well, we don't want to reinvent the wheel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop cookiecutter from molecule
4 participants