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 runtime not being python #745

Closed
wants to merge 6 commits into from
Closed

Fix runtime not being python #745

wants to merge 6 commits into from

Conversation

mklenbw
Copy link
Contributor

@mklenbw mklenbw commented Nov 28, 2022

When runtime is not set to python don't use it as pythonBin.

Closes: #741

@pgrzesik
Copy link
Contributor

Thanks @mklenbw - sorry for not responding sooner, but I've been very busy lately, I'll try to review this PR by the end of the week

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Hey @mklenbw, thanks a lot for your patience, I'm really sorry that I wasn't able to respond sooner to your PR. Do you think you could introduce a test case that covers this case? Please let me know what do you think 🙇

@mklenbw
Copy link
Contributor Author

mklenbw commented Jan 9, 2023

Sure. I'll add it right away and text you.

@ShantanuNair
Copy link

+1 Also facing this issue. Appreciate the PR!

@mklenbw
Copy link
Contributor Author

mklenbw commented Jan 27, 2023

@pgrzesik
Sorry i forgot to push it. Now it's there. Hope this helps.

@pgrzesik
Copy link
Contributor

Hey @mklenbw, thank you so much 🙇 Unforutnatelly it looks like formatting is failing, could you please check that?

@mklenbw
Copy link
Contributor Author

mklenbw commented Feb 3, 2023

@pgrzesik Got it fixed. Forgot to run prettier. You should consider including husky.
There's another problem with the unit-tests. I guess it was the node16.x runtime (might fail on node14 and node12).
The node12-test should be removed as it's EOL since 30.04.2022.

test.js Outdated Show resolved Hide resolved
@pgrzesik
Copy link
Contributor

pgrzesik commented Feb 6, 2023

Hey @mklenbw - thank you, there's a leftover console.log that might be messing up the tests. As for Node12 - modernize the CI matrix on my list of things to do soon 👍

@mklenbw
Copy link
Contributor Author

mklenbw commented Feb 7, 2023

@pgrzesik
Oh sorry. Never worked with tape before and needed some visuals ;-). Thanks for looking around.

@mklenbw
Copy link
Contributor Author

mklenbw commented Feb 8, 2023

@pgrzesik
I'm sorry. I really tried fixing this, but the most test-requirements are so old that my company doesn't allow installation or requirements like perl are just undocumented and unnecessary, that i'm wasting to much time on this.
That means i will quit working on this PR and just take the plugin as broken and outdated as it is.

If someone wants to continue this topic please take my code and get it to work. I would love to see this masterpiece of a plugin in it's full glance again.

@pgrzesik
Copy link
Contributor

pgrzesik commented Feb 8, 2023

No worries, sorry that it became an issue and thanks for all the effors @mklenbw 👍 I'll try to wrap this up over the weekend. I'm trying to keep the lights on with this particular plugin but unfortunately my time has been very limited lately.

@harry-isaac
Copy link

+1 for would love to see this fixed

@jameskbride
Copy link
Contributor

Picked up where @mklenbw left off, see #773 .

@pgrzesik
Copy link
Contributor

pgrzesik commented Aug 1, 2023

thanks @jameskbride - I'll close this one and respond in other PR

@pgrzesik pgrzesik closed this Aug 1, 2023
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.

Plugin destroys deployments with provider.runtime other than python.
5 participants