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: migrate SortManifests and its test case to releaseutil #5236

Merged
merged 1 commit into from Mar 19, 2019

Conversation

lentil1016
Copy link
Contributor

@lentil1016 lentil1016 commented Jan 30, 2019

What this PR does / why we need it:

Hi. I'm currently working on a new feature on branch dev-v3 and I found a TODO in my way to implement this feature.

helm/pkg/action/install.go

Lines 293 to 299 in 86d8596

// Sort hooks, manifests, and partials. Only hooks and manifests are returned,
// as partials are not used after renderer.Render. Empty manifests are also
// removed here.
// TODO: Can we migrate SortManifests out of pkg/tiller?
hooks, manifests, err := releaseutil.SortManifests(files, vs, releaseutil.InstallOrder)
if err != nil {
// By catching parse errors here, we can prevent bogus releases from going

  • merge sortManifests and its test case in pkg/tiller into pkg/releaseutil
  • merge Manifest, manifestFile, result in pkg/tiller into pkg/releaseutil
  • replace the SortByKind in releaseutil
  • remove trivial code in tiller package.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@helm-bot helm-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 30, 2019
@helm-bot helm-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 30, 2019
@lentil1016 lentil1016 changed the title Refactor: migrate SortManifests and its test case to releaseutil [WIP]Refactor: migrate SortManifests and its test case to releaseutil Jan 30, 2019
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 30, 2019
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 30, 2019
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 30, 2019
@lentil1016 lentil1016 changed the title [WIP]Refactor: migrate SortManifests and its test case to releaseutil Refactor: migrate SortManifests and its test case to releaseutil Jan 30, 2019
@bacongobbler bacongobbler added the v3.x Issues and Pull Requests related to the major version v3 label Jan 30, 2019
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 1, 2019
@adamreese adamreese added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2019
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 19, 2019
@lentil1016 lentil1016 closed this Mar 19, 2019
@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 19, 2019
@lentil1016 lentil1016 reopened this Mar 19, 2019
@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 19, 2019
@lentil1016
Copy link
Contributor Author

lentil1016 commented Mar 19, 2019

@adamreese OK, This PR was trying to refactor SortManifests in pkg/tiller package out. But it seems someone has done this refactor and being merged before me. Good job.
Luckily I don't need to close this PR, for he forgot to remove an expired TODO comment. 🤣

Signed-off-by: Sunyk <lentil1016@gmail.com>
@helm-bot helm-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 19, 2019
@bacongobbler bacongobbler merged commit a8d73ff into helm:dev-v3 Mar 19, 2019
@lentil1016 lentil1016 deleted the migrate-sort-manifest branch March 20, 2019 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. v3.x Issues and Pull Requests related to the major version v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants