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

fix #239 , use python-style templating instead of Jinja in the plugins login_cmd_template #240

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

danielpodwysocki
Copy link
Contributor

This file assumes Jinja templating for the port parameter and passes a Jinja-style "{{ port }}".

https://github.com/ansible/molecule/blob/main/src/molecule/command/login.py#L105

When it reaches molecule, it is subsituted in this file and is done by python calling .format() on the string. That causes it to not render correctly and gives users issues running molecule login.

ref: #239

Copy link

Label error. Requires exactly 1 of: bug, enhancement, major, minor, patch, skip-changelog. Found:

@danielpodwysocki
Copy link
Contributor Author

I tested by editing this change into my venv locally and can now log in correctly to ec2 instances by calling molecule login

@danielpodwysocki danielpodwysocki changed the title fix #239 , use python-style templating instead of Jinja in the ec2 login_cmd_template fix #239 , use python-style templating instead of Jinja in the plugins login_cmd_template Feb 18, 2024
@danielpodwysocki
Copy link
Contributor Author

It affects users on all plugins - updated the PR to address each.

hswong3i added a commit to alvistack/ansible-community-molecule-plugins that referenced this pull request Feb 20, 2024
    git clean -xdf
    tar zcvf ../python-molecule-plugins_23.5.3.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp python-molecule-plugins.spec ../python-molecule-plugins_23.5.3-1.spec
    cp ../python*-molecule-plugins*23.5.3*.{gz,xz,spec,dsc} /osc/home\:alvistack/ansible-community-molecule-plugins-23.5.3
    rm -rf ../python*-molecule-plugins*23.5.3*.*

See ansible-community#155
See ansible-community#237
See ansible-community#240

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
@danielpodwysocki
Copy link
Contributor Author

Any news on this?

hswong3i added a commit to alvistack/ansible-community-molecule-plugins that referenced this pull request Feb 28, 2024
    git clean -xdf
    tar zcvf ../python-molecule-plugins_23.5.3.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp python-molecule-plugins.spec ../python-molecule-plugins_23.5.3-1.spec
    cp ../python*-molecule-plugins*23.5.3*.{gz,xz,spec,dsc} /osc/home\:alvistack/ansible-community-molecule-plugins-23.5.3
    rm -rf ../python*-molecule-plugins*23.5.3*.*

See ansible-community#155
See ansible-community#237
See ansible-community#240

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
@danielpodwysocki
Copy link
Contributor Author

Any news on this PR?

This bug breaks some of the plugin functionality quite badly.

Copy link

@cedricverhaeghe cedricverhaeghe left a comment

Choose a reason for hiding this comment

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

I applied these changes locally and can confirm it fixes the issue.

@ekin-oener-provsn
Copy link

Can also confirm, that this fixes the issue.

@ekin-oener-provsn
Copy link

@danielpodwysocki Please add a label to the PR. Would recommend to use 'patch' here. Otherwise following check is failing to success 'ack / ack / ack (pull_request_target) '.
I think, after adding this label, the process of merging will be a step nearer.

@danielpodwysocki
Copy link
Contributor Author

@danielpodwysocki Please add a label to the PR. Would recommend to use 'patch' here. Otherwise following check is failing to success 'ack / ack / ack (pull_request_target) '. I think, after adding this label, the process of merging will be a step nearer.

@ekin-oener-provsn I miss permissions to do that here, I could only create the PR but seems I cannot edit labels. It will need to be someone with enough access to this repo.

@dzintars
Copy link

Confirm. Fixes the same issue for me.

@StarlessNights
Copy link

Fixes the issue on Arch Linux. Seems good to merge.

@cclose
Copy link

cclose commented May 8, 2024

Bump... the fix seems good, we just need a maintainer to add the label to the PR as the submitter asserts they lack permission to edit the labels.

@robpou
Copy link

robpou commented Jun 10, 2024

Bump. Please merge this, the bug is breaking stuff.

@ben16w
Copy link

ben16w commented Jun 21, 2024

Can confirm the PR fixed the issue locally for me, is it possible for it to be merged?

@matt-horwood-mayden
Copy link

please please please get this merged, as we are re-writing all our ansible roles and this would help so much

@isaacfraser-tomtom
Copy link

please please please get this merged, as we are re-writing all our ansible roles and this would help so much

+1 please approve this.

@matt-horwood-mayden
Copy link

@audgirka are you able to progress this PR please?

@apatard apatard added the bug Something isn't working label Jul 23, 2024
@matt-horwood-mayden
Copy link

@apatard Have had a quick look at the failed builds and can see that centos:7 is failing, as thats now EOL should that be removed?

@apatard
Copy link
Member

apatard commented Jul 31, 2024

@apatard Have had a quick look at the failed builds and can see that centos:7 is failing, as thats now EOL should that be removed?

I found time to fix that the other day but didn't find enough time to clean, push and create a PR. I hope I'll do it this week

@apatard
Copy link
Member

apatard commented Aug 2, 2024

PR opened, and looks like the CI is green, so only a reviewer is needed to get it merged. This should unblock this PR

@danielpodwysocki
Copy link
Contributor Author

Any news here?

This is a rather annoying bug to work around and feels like something we should be addressing in weeks, not months?

@apatard
Copy link
Member

apatard commented Aug 22, 2024

Any news here?

This is a rather annoying bug to work around and feels like something we should be addressing in weeks, not months?

once the CI is fixed, this PR will be merged. As long as my PR about fixing the CI is not merged (in this form or any other form), I fear it's stuck :(

@apatard
Copy link
Member

apatard commented Sep 20, 2024

Right, looks like my PR has been merged (big thanks to @ssbarnea). Once this PR is rebased, it'll be ready to be merged I guess.

…nja in all drivers login_cmd_template

This file assumes Jinja templating for the port parameter and passes a Jinja-style "{{ port }}".

https://github.com/ansible/molecule/blob/main/src/molecule/command/login.py#L105

When it reaches molecule, it is subsituted in  this file and is done by python calling `.format()` on the string.
That causes it to not render correctly and gives users issues running
molecule login.

ref: ansible-community#239

Fixed all affected plugins.
@danielpodwysocki
Copy link
Contributor Author

Right, looks like my PR has been merged (big thanks to @ssbarnea). Once this PR is rebased, it'll be ready to be merged I guess.

Awesome - I've rebased off of main.

@apatard apatard merged commit fdc748f into ansible-community:main Sep 27, 2024
10 checks passed
@simi
Copy link

simi commented Dec 13, 2024

Any chance to finally release? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.