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

Add optional support for helm diff #355

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

gravesm
Copy link
Member

@gravesm gravesm commented Jan 27, 2021

SUMMARY

There are some cases where the existing module has difficulty
determining if an upgrade would result in changes. This can particularly
be a problem when changes are made to a local chart.

This adds optional support for helm diff. If the plugin is present it
will be used. Otherwise, the default implementation will be used and a
warning will be issued. One caveat: helm diff does not currently support
using a repo url, so the default implementation will be used in this
case, as well.

Closes #248

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

helm

ADDITIONAL INFORMATION

Several of our existing tests break when helm diff is installed. This is because our local path test charts don't actually contain any resources so nothing changes when we do any upgrade. Changing the appVersion or adding new values won't make a difference. I have added a simple ConfigMap to our test charts so that when an upgrade happens it will actually change something. This is the reason for all the changes to the existing tests.

There are some cases where the existing module has difficulty
determining if an upgrade would result in changes. This can particularly
be a problem when changes are made to a local chart.

This adds optional support for helm diff. If the plugin is present it
will be used. Otherwise, the default implementation will be used and a
warning will be issued. One caveat: helm diff does not currently support
using a repo url, so the default implementation will be used in this
case, as well.
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #355 (5a35725) into main (5a5ed79) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #355   +/-   ##
=======================================
  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 5a5ed79...5a35725. Read the comment docs.

@Akasurde
Copy link
Member

ERROR: plugins/modules/helm.py:412:14: blacklisted-name: Black listed name "_"

@gravesm
Copy link
Member Author

gravesm commented Jan 28, 2021

@Akasurde yes, that one I'm not worried about. It's an easy fix. The bigger problem is I don't know how to address the yamllint errors. Fixing those errors will break the templates for go.

@Akasurde
Copy link
Member

Oh yes. I didn't see that. I will check that now.

@gravesm
Copy link
Member Author

gravesm commented Jan 28, 2021

@Akasurde I think I sorted out the linting issues. If there's a better way to do this, I'm happy to be educated. I wish we could ignore a directory, but that doesn't seem possible.

@Akasurde
Copy link
Member

@gravesm We can disable yamllint with several ways.

  1. Disable directory - https://yamllint.readthedocs.io/en/stable/configuration.html#ignoring-paths
  2. Disable single line - https://yamllint.readthedocs.io/en/stable/disable_with_comments.html#disabling-checks-for-a-specific-line
  3. Disable single file - https://yamllint.readthedocs.io/en/stable/disable_with_comments.html#disabling-checks-for-all-or-part-of-the-file

I am OK with current configuration as well since I don't see chart files changing any time soon.

Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

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

LGTM

@Akasurde Akasurde merged commit 2640084 into ansible-collections:main Feb 3, 2021
@Akasurde Akasurde added has_issue This PR has a related issue it could close. type/enhancement New feature or request labels Feb 3, 2021
@gravesm gravesm deleted the 248-helm-diff branch February 19, 2021 17:57
@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.
Labels
has_issue This PR has a related issue it could close. repo-lockdown type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants