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

helm: Add option 'atomic' #115

Merged
merged 1 commit into from
Jun 18, 2020
Merged

helm: Add option 'atomic' #115

merged 1 commit into from
Jun 18, 2020

Conversation

Akasurde
Copy link
Member

@Akasurde Akasurde commented Jun 2, 2020

SUMMARY

helm command provides option to remove installation on failure using
'atomic' flag.
This fix adds this parameter in helm module.

Fixes: #109

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/modules/helm.py

@codecov
Copy link

codecov bot commented Jun 2, 2020 •
edited
Loading

Codecov Report

Merging #115 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #115   +/-   ##
=======================================
  Coverage   42.56%   42.56%           
=======================================
  Files           3        3           
  Lines         545      545           
  Branches      110      110           
=======================================
  Hits          232      232           
  Misses        270      270           
  Partials       43       43

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 a9aab12...5b70464. Read the comment docs.

@Akasurde Akasurde mentioned this pull request Jun 2, 2020
Closed
@abhiTamrakar
Copy link

abhiTamrakar commented Jun 2, 2020 •
edited
Loading

@Akasurde appreciate your help. I didn't get time today to validate this one, but looking at the change, seems good to me.
Though I am not a reviewer here, but just a suggestion, would it be better to call module.run_command(args) once instead of calling it in all functions. The **kwargs can be used to gather all options and then based on the value of state, the commands shall be formed and run at the end, except got getting the status. That ways there will be uniformity in the module.

@Akasurde
Copy link
Member Author

Akasurde commented Jun 3, 2020

@abhiTamrakar It is being handled in this PR #110

@fabianvf
Copy link
Collaborator

fabianvf commented Jun 4, 2020

@Akasurde merged #110, would you mind rebasing this one?

fabianvf
fabianvf reviewed Jun 8, 2020
View reviewed changes
Copy link
Collaborator

@fabianvf fabianvf left a comment

Choose a reason for hiding this comment

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

Other than one nit looks great

plugins/modules/helm.py Show resolved Hide resolved
willthames
willthames previously requested changes Jun 9, 2020
View reviewed changes
plugins/modules/helm.py Outdated Show resolved Hide resolved
@Akasurde Akasurde force-pushed the i109 branch 2 times, most recently from 5525681 to 71b1558 Compare June 9, 2020 11:02
@Akasurde Akasurde force-pushed the i109 branch 3 times, most recently from 1fac6ff to 48eecee Compare June 18, 2020 10:57
@Akasurde
helm: Add option 'atomic'
5b70464 
helm command provides option to remove installation on failure using
'atomic' flag.
This fix adds this parameter in helm module.

Fixes: ansible-collections#109

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@geerlingguy geerlingguy dismissed willthames’s stale review June 18, 2020 20:14

Change was made, we are good to go.

@geerlingguy geerlingguy merged commit e6773fd into ansible-collections:master Jun 18, 2020
This was referenced Jun 18, 2020
Closed
Closed
@Akasurde Akasurde deleted the i109 branch June 19, 2020 01:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@willthames willthames willthames left review comments

@fabianvf fabianvf fabianvf approved these changes

@geerlingguy geerlingguy geerlingguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

include an optional --atomic flag with helm install and upgrade
5 participants
@Akasurde @abhiTamrakar @fabianvf @willthames @geerlingguy