Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Remove component in module with#796

Merged
bryanl merged 1 commit intoksonnet:masterfrom
bryanl:remove-component
Jul 30, 2018
Merged

Remove component in module with#796
bryanl merged 1 commit intoksonnet:masterfrom
bryanl:remove-component

Conversation

@bryanl
Copy link
Member

@bryanl bryanl commented Jul 30, 2018

Removes component in module with ks component rm module.name. The previous behavior did not respect the module name.

#795

Signed-off-by: bryanl bryanliles@gmail.com

@bryanl bryanl self-assigned this Jul 30, 2018
@bryanl bryanl requested a review from a team July 30, 2018 12:46
@bryanl bryanl closed this Jul 30, 2018
@bryanl bryanl reopened this Jul 30, 2018
@coveralls
Copy link

coveralls commented Jul 30, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 1249

  • 26 of 40 (65.0%) changed or added relevant lines in 6 files are covered.
  • 18 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 71.07%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/component/component.go 1 2 50.0%
pkg/component/jsonnet.go 5 7 71.43%
pkg/component/delete.go 18 22 81.82%
pkg/component/yaml.go 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/component/component.go 18 15.63%
Totals Coverage Status
Change from base Build 1247: -0.1%
Covered Lines: 11971
Relevant Lines: 16844
💛 - Coveralls

@shomron
Copy link
Collaborator

shomron commented Jul 30, 2018

Looks good, no blockers.

There are a two edge cases we may want to address in followups -

  • Missing component file (foo.jsonnet) causes failure, but missing component reference in params.jsonnet does not. Do we want to gracefully recover so that when one or the other is out of sync the user can still remove the remaining parts?
  • If a component is ambiguous (foo.jsonnet, foo.yaml both exist), one will be arbitrarily removed and the other will remain

@bryanl bryanl mentioned this pull request Jul 30, 2018
Open
@bryanl
Copy link
Member Author

bryanl commented Jul 30, 2018

@shomron I created #799 to track the points you brought up.

@bryanl
Remove component in module with
d617aed 
Removes component in module with `ks component rm module.name`

Signed-off-by: bryanl <bryanliles@gmail.com>
@bryanl bryanl force-pushed the remove-component branch from e440273 to d617aed Compare July 30, 2018 17:47
@bryanl bryanl merged commit b6b33c5 into ksonnet:master Jul 30, 2018
@bryanl bryanl deleted the remove-component branch July 30, 2018 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@shomron shomron shomron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bryanl bryanl

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryanl @coveralls @shomron