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.

Sign up for GitHub

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

Jump to bottom

Remove and prevent unchecked type assertions #1245

Merged
merged 4 commits into from Jan 8, 2020
Merged

Remove and prevent unchecked type assertions #1245

merged 4 commits into from Jan 8, 2020

Conversation

porridge
Copy link
Member

@porridge porridge commented Jan 3, 2020 •
edited

What this PR does / why we need it:

  • also add a test for the changed code in LoadYAML
  • refactor
  • add errcheck to linters and make it report unchecked type assertions.

Fixes: #646

@porridge porridge requested a review from nfnt January 3, 2020 15:53
@porridge porridge mentioned this pull request Jan 3, 2020
Closed
@porridge
Copy link
Member Author

porridge commented Jan 3, 2020 •
edited

Whoops, looks like I'll have to put these YAML files in a different directory to prevent TestHarnessRunIntegration/harness from trying to load them... I'll do that on Tuesday.

@porridge porridge changed the title Use checked type assertions in LoadYAML. Remove and prevent unchecked type assertions Jan 7, 2020
zen-dog
zen-dog approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

The profit from enabling the type-assertions check seems to be minuscular. We added some checks for little actual benefit. Approved tentatively 🤷‍♀

.golangci.yml
@@ -1,6 +1,7 @@
linters:
auto-fix: false
enable:
- errcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

It is enabled by default but being explicit is fine

pkg/engine/task/task_pipe.go
@@ -104,7 +104,10 @@ func (pt PipeTask) Run(ctx Context) (bool, error) {
// 8. - Copy out the pipe files -
log.Printf("PipeTask: %s/%s copying pipe files", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName)
fs := afero.NewMemMapFs()
pipePod := podObj[0].(*corev1.Pod)
pipePod, ok := podObj[0].(*corev1.Pod)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should never happen since we create that same corev1.Pod a few lines above it

pkg/test/step.go
@@ -451,7 +455,11 @@ func (s *Step) LoadYAML(file string) error {

for _, obj := range s.Apply {
if obj.GetObjectKind().GroupVersionKind().Kind == "TestStep" {
s.Step = obj.(*kudo.TestStep)
if testStep, ok := obj.(*kudo.TestStep); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

The kind is tested in the line above

Copy link
Member Author

@porridge porridge Jan 7, 2020 •
edited

Choose a reason for hiding this comment

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

Right, but that's orthogonal to the type, as the added test shows. This is the crux of the issue that this PR Fixes:.

pkg/test/utils/subset.go
subsetErr := err.(*SubsetError)
subsetErr.AppendPath(iter.Key().String())
return subsetErr
subsetErr, ok := err.(*SubsetError)
Copy link
Contributor

Choose a reason for hiding this comment

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

IsSubset seems to return only SubsetErrors for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. Do you think we should change its signature to return *SubsetErrors instead?

@porridge porridge merged commit e492fc7 into master Jan 8, 2020
@porridge porridge deleted the check-type-assertions branch January 8, 2020 09:45
@porridge porridge mentioned this pull request Jan 8, 2020
Merged
porridge added a commit that referenced this pull request Jan 8, 2020
@porridge
Check the type assertion on funk.Keys return value. (#1256)
6dcbee3 
Make lint happy again.
This was result of a race between merging #1223 and #1245
ANeumann82 pushed a commit that referenced this pull request Feb 13, 2020
@porridge @ANeumann82
Remove and prevent unchecked type assertions (#1245)
58a90c4 
* Enable errcheck with check-type-assertions.
* Also add a test for the checked type assertions in LoadYAML.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82 pushed a commit that referenced this pull request Feb 13, 2020
@porridge @ANeumann82
Check the type assertion on funk.Keys return value. (#1256)
ee84900 
Make lint happy again.
This was result of a race between merging #1223 and #1245

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zen-dog zen-dog zen-dog approved these changes

@jbarrick-mesosphere jbarrick-mesosphere Awaiting requested review from jbarrick-mesosphere jbarrick-mesosphere is a code owner

@nfnt nfnt Awaiting requested review from nfnt nfnt is a code owner

@alenkacz alenkacz Awaiting requested review from alenkacz alenkacz is a code owner

@gerred gerred Awaiting requested review from gerred gerred is a code owner

@kensipe kensipe Awaiting requested review from kensipe kensipe is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Get rid of unchecked type assertions
2 participants
@porridge @zen-dog