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

requested changes in https://github.com/k8snetworkplumbingwg/kubemacpool/pull/100#discussion_r388329464 #106

Open
alonSadan opened this issue Mar 5, 2020 · 2 comments

Comments

@alonSadan
Copy link
Contributor

alonSadan commented Mar 5, 2020

#100
in order to keeo track on tohse notes I'm adding them here:

Expect(err).ToNot(HaveOccurred())

  • Maybe we can keep the failure message, here and and in the rest of the Expects. something like :
    Expect(err).ToNot(HaveOccurred(), "failed to apply the new vm object")

Expect(strings.Contains(err.Error(), "failed to allocate requested mac address")).To(Equal(true))

  • maybe check that err is not nil, before checking that the error is as expected?
Expect(err).To(HaveOccurred())
Expect(strings.Contains(err.Error(), "failed to allocate requested mac address")).To(Equal(true))
@phoracek
Copy link
Member

phoracek commented Mar 5, 2020

Could you also share the code you are commenting on? Ideally pinned to a specific revision, like following:

*.out

https://github.com/k8snetworkplumbingwg/kubemacpool/blob/75371d2d5442ac33f56a8500f58d6bdce19805d5/.gitignore#L14

All the needed information should be in this Issue.

@alonSadan
Copy link
Contributor Author

sure. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants