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

Refactor helm init command for local helm home directory initialization reuse #5189

Merged
merged 1 commit into from Mar 12, 2019

Conversation

Projects
None yet
4 participants
@pdecat
Copy link
Contributor

pdecat commented Jan 19, 2019

What this PR does / why we need it:

This PR refactors the helm init command to allow other programs to initialize the local helm home directory without shelling out to the helm init command.

The only moved parts are:

  • ensureDirectories()
  • ensureDefaultRepos()
  • ensureRepoFileFormat()

My current use case is with the terraform-provider-helm to resolve terraform-providers/terraform-provider-helm#139

Special notes for your reviewer:

I've prefixed the PR title as WIP because the terraform provider part is not yet implemented (in progress) but I wanted to check early if this sort of change would be acceptable on the helm end.

If applicable:

  • this PR contains documentation (N/A: no CLI user visible change)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility (N/A: no structure change)

Unit test results:

# go test -v ./cmd/helm ./cmd/helm/installer -run '^Test(InitCmd|EnsureHome|Initialize)' -count=1
=== RUN   TestInitCmd
--- PASS: TestInitCmd (50.96s)
=== RUN   TestInitCmd_exists
--- PASS: TestInitCmd_exists (87.33s)
=== RUN   TestInitCmd_clientOnly
--- PASS: TestInitCmd_clientOnly (4.09s)
=== RUN   TestInitCmd_dryRun
--- PASS: TestInitCmd_dryRun (0.00s)
=== RUN   TestInitCmd_tlsOptions
--- PASS: TestInitCmd_tlsOptions (0.00s)
=== RUN   TestInitCmd_output
--- PASS: TestInitCmd_output (0.00s)
PASS
ok      k8s.io/helm/cmd/helm    142.437s
=== RUN   TestInitialize
--- PASS: TestInitialize (52.90s)
=== RUN   TestEnsureHome
--- PASS: TestEnsureHome (36.85s)
PASS
ok      k8s.io/helm/cmd/helm/installer  89.772s

@helm-bot helm-bot added the size/L label Jan 19, 2019

@pdecat pdecat force-pushed the pdecat:refactor-init-for-reuse branch from d38f44d to beb5e63 Jan 19, 2019

@helm-bot helm-bot added size/L and removed size/L labels Jan 19, 2019

@pdecat pdecat force-pushed the pdecat:refactor-init-for-reuse branch from beb5e63 to 6eefd6d Jan 19, 2019

@helm-bot helm-bot added size/L and removed size/L labels Jan 19, 2019

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Jan 19, 2019

Note: backported on v2.11.0 for easier testing on terraform-provider-helm: https://github.com/pdecat/helm/tree/v2.11.0-backport-5189

Show resolved Hide resolved cmd/helm/init.go

@pdecat pdecat changed the title WIP: Refactor helm init command for local helm home directory initialization reuse Refactor helm init command for local helm home directory initialization reuse Jan 19, 2019

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Jan 19, 2019

Removed the WIP prefix as it's working fine: terraform-providers/terraform-provider-helm#185

@bacongobbler

This comment has been minimized.

Copy link
Member

bacongobbler commented Feb 1, 2019

Nice refactor! Code here looks good to me. Would you mind rebasing this?

@pdecat pdecat force-pushed the pdecat:refactor-init-for-reuse branch from 6eefd6d to 7e07826 Feb 2, 2019

@helm-bot helm-bot added size/L and removed size/L labels Feb 2, 2019

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Feb 2, 2019

@bacongobbler thanks!

Sure, rebased.

@bacongobbler
Copy link
Member

bacongobbler left a comment

Needs another maintainer to sign off since this is a size/L, but this looks good to merge.

In the meantime, if you want to change Initialize to EnsureHome then that'd be great, but it's not blocking this from going through.

@pdecat pdecat force-pushed the pdecat:refactor-init-for-reuse branch from 7e07826 to e9a8aaf Feb 5, 2019

@helm-bot helm-bot added size/L and removed size/L labels Feb 5, 2019

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Feb 12, 2019

Hi @bacongobbler, 10 days have passed since you approved this PR, how can this be moved further? Am I supposed to ping another maintainer? Should I rebase once again?

Refactor helm init command for reuse, allowing other programs to init…
…ialize local helm home directory without shelling out to the helm binary

Signed-off-by: Patrick Decat <pdecat@gmail.com>

@pdecat pdecat force-pushed the pdecat:refactor-init-for-reuse branch from e9a8aaf to 7da53d6 Mar 6, 2019

@helm-bot helm-bot added size/L and removed size/L labels Mar 6, 2019

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Mar 6, 2019

Rebased on master.

@bacongobbler

This comment has been minimized.

Copy link
Member

bacongobbler commented Mar 12, 2019

Yes, please try reaching out to another maintainer to see if they have the time to review it.

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Mar 12, 2019

Hi @technosophos @mattfarina, as the most active maintainers here apart from @bacongobbler who already reviewed and approved this PR, could you please take a look at it?

@technosophos
Copy link
Member

technosophos left a comment

Reviewed code, and checked specifically for compatibility breakers. All looks great. @bacongobbler feel free to merge at your convenience.

@bacongobbler bacongobbler added this to the 2.14.0 milestone Mar 12, 2019

@bacongobbler bacongobbler merged commit 87b44dd into helm:master Mar 12, 2019

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@pdecat pdecat deleted the pdecat:refactor-init-for-reuse branch Mar 12, 2019

@pdecat

This comment has been minimized.

Copy link
Contributor Author

pdecat commented Mar 12, 2019

Thank you so much @bacongobbler & @technosophos !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.