salt icon indicating copy to clipboard operation
salt copied to clipboard

Fix #62281

Open itspooya opened this issue 2 years ago • 2 comments

What does this PR do?

added if-else block to check the python version and fill repo_path with correct data regarding the python major version

What issues does this PR fix or reference?

Fixes: #62281

itspooya avatar Jul 12 '22 21:07 itspooya

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey. Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

welcome[bot] avatar Jul 12 '22 21:07 welcome[bot]

Second commit's code has better performance than if-else's code block repo_path = os.getcwd() if sys.version_info.major == 2: repo_path = os.getcwdu()

itspooya avatar Jul 12 '22 21:07 itspooya

this is going to need a testcase and changelog.

whytewolf avatar Oct 05 '22 19:10 whytewolf

Thank you for your comment I will add following changes(test case and changelog)

itspooya avatar Oct 05 '22 21:10 itspooya

@whytewolf Could you please take a look at this PR? I would be happy to make any changes needed to this PR since it's been open for a while

itspooya avatar Nov 07 '22 17:11 itspooya

did you see the comment about python2 not being supported? I would be happier if we were not importing python2 code.

whytewolf avatar Nov 07 '22 20:11 whytewolf

@whytewolf

did you see the comment about python2 not being supported? I would be happier if we were not importing python2 code.

Sorry, I haven't seen that comment, I removed Python-2 code and sent another commit to remove it Could you check it now?

itspooya avatar Nov 07 '22 20:11 itspooya

looks like there is a precommit issue. fix that and I'll approve again.

whytewolf avatar Nov 07 '22 21:11 whytewolf

@whytewolf Hi again Sorry for taking your time too much, it's my first time contributing to a big opensource project I think this time I got it right

itspooya avatar Nov 07 '22 22:11 itspooya

looks like the precommit is still failing.

you might need to install it locally and see if you can get it to pass.

What I normally do is

git add .
precommit
git add .
precommit

the second one should pass unless there is something that the precommit doesn't fix automatically.

whytewolf avatar Nov 07 '22 22:11 whytewolf

@whytewolf Thank you for guiding me I got it right this time by setting up local environment Please check this Thanks

itspooya avatar Nov 07 '22 23:11 itspooya

@whytewolf Thank you for guiding me I got it right this time by setting up local environment Please check this Thanks

yeap. approved. pending tests now.

whytewolf avatar Nov 08 '22 00:11 whytewolf

@whytewolf That one test I think it needs to re run?

itspooya avatar Nov 08 '22 01:11 itspooya

@whytewolf Everything is done and checks passed

itspooya avatar Nov 08 '22 06:11 itspooya

@whytewolf Just wanted to check whether everything is okay

itspooya avatar Nov 08 '22 15:11 itspooya

@whytewolf Just wanted to check whether everything is okay

yeap things are looking good. I"ll get this in front of the team today to get merged.

whytewolf avatar Nov 08 '22 17:11 whytewolf

Congratulations on your first PR being merged! :tada:

welcome[bot] avatar Nov 08 '22 19:11 welcome[bot]