-
Notifications
You must be signed in to change notification settings - Fork 174
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
vic-machine validator bail out if session populate doesn't find a vm folder #7491
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -359,6 +359,13 @@ func (s *Session) Populate(ctx context.Context) (*Session, error) { | |
} else { | ||
op.Debugf("Cached folders: %s", s.DatacenterPath) | ||
} | ||
// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out. | ||
// It's also possible that there's an error, but a valid vm folder is returned | ||
if folders == nil { | ||
errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath)) | ||
return nil, errors.New(strings.Join(errs, "\n")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just omit this here, and rely on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. under some circumstances (tho very rare), Also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I'm suggesting anything that'd require iterating. Right now, you have: if s.Datacenter != nil {
folders, err := s.Datacenter.Folders(op)
if err != nil {
errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
} else {
op.Debugf("Cached folders: %s", s.DatacenterPath)
}
// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
// It's also possible that there's an error, but a valid vm folder is returned
if folders == nil {
errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
return nil, errors.New(strings.Join(errs, "\n"))
}
s.VMFolder = folders.VmFolder
}
if len(errs) > 0 {
op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
return nil, errors.New(strings.Join(errs, "\n"))
}
op.Debug("vSphere resource cache populated...")
return s, nil I think you could just remove the return you added without changing the behavior: if s.Datacenter != nil {
folders, err := s.Datacenter.Folders(op)
if err != nil {
errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
} else {
op.Debugf("Cached folders: %s", s.DatacenterPath)
}
// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
// It's also possible that there's an error, but a valid vm folder is returned
if folders == nil {
errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
- return nil, errors.New(strings.Join(errs, "\n"))
- }
-
+ } else {
s.VMFolder = folders.VmFolder
+ }
}
if len(errs) > 0 {
op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
return nil, errors.New(strings.Join(errs, "\n"))
}
op.Debug("vSphere resource cache populated...")
return s, nil The behavior should still be the same; since you're appending to if s.Datacenter != nil {
folders, err := s.Datacenter.Folders(op)
if err != nil {
errs = append(errs, fmt.Sprintf("Failure finding folders (%s): %s", s.DatacenterPath, err.Error()))
} else {
op.Debugf("Cached folders: %s", s.DatacenterPath)
}
// There could be cases where no error from Datacenter.Folders, but nil folder is returned. In this case we should bail out.
// It's also possible that there's an error, but a valid vm folder is returned
if folders == nil {
errs = append(errs, fmt.Sprintf("Nil folder returned when finding folders (%s)", s.DatacenterPath))
} else {
s.VMFolder = folders.VmFolder
}
}
if len(errs) > 0 {
op.Debugf("Error count populating vSphere cache: (%d)", len(errs))
return nil, errors.New(strings.Join(errs, "\n"))
}
op.Debug("vSphere resource cache populated...")
return s, nil This essentially just preserves the pattern the code was already using: accumulate a collection of errors and then return all of them. This is important because if someone adds another check after the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what I wanted was to make the session.Populate early abort when Preserving the error accumulation pattern makes perfect sense. To make it cleaner, I think I could do sth like this:
And then it's the validator's job to check |
||
} | ||
|
||
s.VMFolder = folders.VmFolder | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may cause issues when doing a
vic-machine ls
with multiple datacenters.I suggest delaying any further work on this until @matthewavery has delivered the inventory folder support (#773) and then revisting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. I will then leave the PR as it is for now, and move the ticket back to To-Do. Thanks!