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

Implement cleanup of temporary files after execution #62

Merged
merged 3 commits into from
Sep 5, 2021

Conversation

dm3ch
Copy link
Contributor

@dm3ch dm3ch commented Jul 19, 2021

Implement cleanup of temporary helmfiles after execution

/fix #59

…helmfile_release_set to NewCommandWithKubeconfig of helmfile_release_set. This is a function that actually executes helmfile binary.
@dm3ch dm3ch marked this pull request as draft July 19, 2021 11:02
@dm3ch dm3ch force-pushed the cleanup branch 2 times, most recently from 7182039 to 34dba79 Compare July 19, 2021 13:54
…t used temporarty helmfile path.

Handle tmpHelmfilPath removal in all places where `NewCommandWithKubeconfig` executed.
Unify path for temporart helmfile and other tmp provider resources.
… cause it causes issue that dir not exist.

Making provider delete tmp directory after run
@dm3ch dm3ch marked this pull request as ready for review July 19, 2021 14:08
@dm3ch
Copy link
Contributor Author

dm3ch commented Jul 19, 2021

@mumoshu PR is ready for review.

@dm3ch
Copy link
Contributor Author

dm3ch commented Jul 19, 2021

@mumoshu PR is ready for review.

Each change and it's reason described in commit messages

@dm3ch dm3ch changed the title Implement cleanup of temporary helmfiles after execution Implement cleanup of temporary files after execution Jul 19, 2021
@dm3ch
Copy link
Contributor Author

dm3ch commented Sep 3, 2021

Any chance to get it merged?

// panic: interface conversion: interface {} is nil, not string
if path := d.Get(KeyPath); path != nil {
f.Path = path.(string)
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to merge this and make necessary changes(if any) later but one thing to ask here- What was the rationale behind removing this feature (specifying an existing path) in this pull request?

Copy link
Contributor Author

@dm3ch dm3ch Sep 6, 2021

Choose a reason for hiding this comment

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

@mumoshu When I started finding root cause of persisting tmp files, i found that there's two separate pieces of code creating tmp files for releases:

  1. Creation of helmfile for standalone helm release
  2. Copying existing helmfile before apply

This cases had completely different places where tmp files were created, so I refactored both cases to pass helmfile content same way and make same tmp file creation code for them.

When I sent this PR I tested that both terraform resources works fine with this PR, so my idea was unification of tmp file creation without breaking how terraform resources works

Copy link
Owner

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

Mostly LGTM with only one question! Thanks for your contribution @dm3ch

@mumoshu mumoshu merged commit 36200a6 into mumoshu:master Sep 5, 2021
@yuriipolishchuk
Copy link

yuriipolishchuk commented Sep 16, 2021

@mumoshu may we have a new release with this improvement, please?

@dm3ch thank you 👍

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.

Cleanup of temporary helmfiles
3 participants