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

Provide minimal test for whereabouts, along with successful vendor of the net-attach-def client for k8s 1.22 #197

Conversation

maiqueb
Copy link
Collaborator

@maiqueb maiqueb commented Feb 22, 2022

Worked on top of PR #196.

@maiqueb maiqueb force-pushed the vendor-nad-stuff-along-with-minimal-test branch 3 times, most recently from e84a760 to 13794f7 Compare February 22, 2022 17:41
@maiqueb maiqueb changed the title Provide minimal test for whereabouts, along with successful vendor of the net-attach-def client for k8s >= 1.22 Provide minimal test for whereabouts, along with successful vendor of the net-attach-def client for k8s 1.22 Feb 22, 2022
@coveralls
Copy link

coveralls commented Feb 22, 2022

Pull Request Test Coverage Report for Build 1910049182

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 40.548%

Totals Coverage Status
Change from base Build 1894833668: 0.0%
Covered Lines: 444
Relevant Lines: 1095

💛 - Coveralls

@maiqueb maiqueb force-pushed the vendor-nad-stuff-along-with-minimal-test branch 4 times, most recently from f743322 to a11cacb Compare February 23, 2022 10:42
Copy link
Collaborator

@nicklesimba nicklesimba left a comment

Choose a reason for hiding this comment

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

I added a few questions and also changes I'd like to see. Feedback on my feedback welcome :) Overall looks great!

e2e/pod_status.go Outdated Show resolved Hide resolved
e2e/e2e_test.go Outdated Show resolved Hide resolved
e2e/e2e_test.go Show resolved Hide resolved
@maiqueb maiqueb force-pushed the vendor-nad-stuff-along-with-minimal-test branch 2 times, most recently from 8953a49 to 4af5833 Compare February 23, 2022 16:43
Copy link
Collaborator

@nicklesimba nicklesimba left a comment

Choose a reason for hiding this comment

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

LGTM, @dougbtv can you give this a pass when you get a chance as well?

@nicklesimba nicklesimba requested a review from dougbtv February 23, 2022 18:36
@maiqueb maiqueb mentioned this pull request Feb 23, 2022
@nicklesimba nicklesimba mentioned this pull request Feb 24, 2022
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

This is a solid LGTM. Seems like you guys got this squeaky clean before the submission.

if n.CNIVersion == v {
return fmt.Errorf("CNI version %v does not support more than 1 address per family", n.CNIVersion)
}
if isSupportedVersion, _ := version.GreaterThanOrEqualTo(n.CNIVersion, minimumSupportedVersion); !isSupportedVersion {
Copy link
Member

Choose a reason for hiding this comment

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

Much nicer!


// WaitForPodBySelector waits up to timeout seconds for all pods in 'namespace' with given 'selector' to enter provided state
// If no pods are found, return nil.
func WaitForPodBySelector(cs *kubernetes.Clientset, namespace, selector string, timeout int) error {
Copy link
Member

Choose a reason for hiding this comment

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

Do we call this anywhere? We don't have to remove it, totally ok to have, just making sure I'm reading it correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! This was left in as part of the more complex e2e test that I'm working on. We can keep it in if it's not a big deal since it will get used eventually, but I'm okay with removing it, too.

Copy link
Member

Choose a reason for hiding this comment

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

No prob at all. Let's keep it!

@maiqueb
Copy link
Collaborator Author

maiqueb commented Feb 25, 2022

/hold

@dougbtv I'm having some doubts about the proposed vendoring here - for whatever reason I am not being able to get this to work with #185. Give me a while to figure this one out...

I'll also rebase the PR with latest master.

@maiqueb maiqueb force-pushed the vendor-nad-stuff-along-with-minimal-test branch from b0210d4 to e94f447 Compare February 25, 2022 08:53
maiqueb and others added 5 commits February 28, 2022 11:46
Signed-off-by: Miguel Duarte Barroso <[email protected]>
Pass the path to the kubeconfig, and cleanup some variables that are not
in use.

Also, ensure that the e2e tests are not executed when the
`hack/test-go.sh` script is executed - that is for the unit tests only.

Signed-off-by: Miguel Duarte Barroso <[email protected]>
This commit is an effort at improving the code readability, while
preserving its functionality.

It does:
  - provide properly scoped variables
  - adds some helper functions to improve readability
  - removes `Expect`ations from the helper functions
  - remove redundant comments - they're just one more thing to maintain
  - use receiver functions when appropriate (aka methods)

Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb maiqueb force-pushed the vendor-nad-stuff-along-with-minimal-test branch from e94f447 to 9b0d073 Compare February 28, 2022 11:46
@dougbtv dougbtv merged commit f30fb87 into k8snetworkplumbingwg:master Feb 28, 2022
@maiqueb maiqueb deleted the vendor-nad-stuff-along-with-minimal-test branch February 28, 2022 15:50
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

Successfully merging this pull request may close these issues.

4 participants