Skip to content
This repository has been archived by the owner on Jun 13, 2024. It is now read-only.

make has_plugin detection method more reliable #362

Merged
merged 6 commits into from
Feb 16, 2021

Conversation

joschi36
Copy link
Contributor

@joschi36 joschi36 commented Feb 4, 2021

SUMMARY

This PR makes the plugin detection method more reliable.
The issue before was that when you had multiple plugins it wouldn't split at the right point. This method just loops through the list and searches the start of the string for the plugin name.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

helm

ADDITIONAL INFORMATION

Why the method before was a problem: With multiple plugins with different plugin name lengths installed the detection method failed.

NAME    	VERSION       	DESCRIPTION                                                                         
diff    	3.1.3         	Preview helm upgrade changes as a diff                                              
helm-git	0.8.1         	Get non-packaged Charts directly from Git.                                          
unittest	0.1.6-rancher1	Unit test for helm chart in YAML with ease to keep your chart functional and robust.

So my line is diff\s\s\s\s\s\t3.1.3[...] and the result variable name with the behavior before was diff\s\s\s\s\s which unfortunately wasn't equal to ´diff´
I hope I could explain it well, If you have an additional question please ask.

/cc @gravesm

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #362 (72b7772) into main (92e3732) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #362   +/-   ##
=======================================
  Coverage   36.80%   36.80%           
=======================================
  Files           3        3           
  Lines         758      758           
  Branches      148      148           
=======================================
  Hits          279      279           
  Misses        430      430           
  Partials       49       49           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92e3732...72b7772. Read the comment docs.

@gravesm
Copy link
Member

gravesm commented Feb 4, 2021

@joschi36 thank you for the PR. I think the better solution here would be to just change it to line.split(None, 1). If you searched for a plugin named foo but had one named foobar installed then using startswith("foo") would lead to a false positive.

Also, could you add a changelog fragment? https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment

@joschi36
Copy link
Contributor Author

joschi36 commented Feb 4, 2021

Thanks for the feedback. I first also thought on doing that, however I choose to use the more simple variant. But yes you are totally right. I will update the PR with this change and also with the change log fragment.

@joschi36
Copy link
Contributor Author

joschi36 commented Feb 4, 2021

@gravesm I have updated the PR

@@ -0,0 +1,2 @@
bugfixes:
- helm - make helm-diff plugin detection more reliable by splitting by any whitespace instead of explicit whitespace (`\s`)
Copy link
Member

Choose a reason for hiding this comment

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

Just a couple minor nits. Since this gets turned into rst those should be double backticks. Could you also add the link to this PR at the end in parentheses? There are a few examples here: https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment.

Thanks for your work on this!

@joschi36
Copy link
Contributor Author

@gravesm I have fixed the double backticks.

Copy link
Member

@gravesm gravesm left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @joschi36!

@github-actions
Copy link

github-actions bot commented Nov 4, 2021

This repository does not accept pull requests, see the README for details.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants