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

chore: Small style + usability refactors for new contributors #86

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

rakechill
Copy link
Contributor

@rakechill rakechill commented Jan 8, 2024

Description
These changes are based on feedback from @matthchr

  • Update (and as a follow-up git ignore) skaffold.yaml so that make az-all automated local changes during development are less confusing.
  • Style refactors
    • NewAZClientFromAPI vs. NewAZClient. Maybe one of these should call the other then? NewAZClient constructs all the clients + calls NewAZClientFromAPI
    • Inconsistent with abbreviation casing (CreateAzClient vs. NewAZClient)
    • Fakes: var _ instance.NetworkInterfacesAPI = (*NetworkInterfacesAPI)(nil) ->
      var _ instance.NetworkInterfacesAPI = &NetworkInterfacesAPI{}

How was this change tested?

  • make presubmit
  • make az-all

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

@Bryce-Soghigian
Copy link
Collaborator

Bryce-Soghigian commented Jan 9, 2024

@rakechill there is more than a single test using normal go tests.

./pkg/auth/cred_test.go
./pkg/auth/config_test.go
./pkg/providers/imagefamily/bootstrap/aksbootstrap_test.go
./pkg/fake/skus_test.go
./pkg/fake/azureresourcegraphapi_test.go
./pkg/utils/gpu_test.go

Are all the go tests. IMO sometimes its nice to write a very simple go test rather than setting up all the ginkgo shenanigans and they are better for fine grained unit tests vs the coarse unit tests we write through the fakes framework.

@Bryce-Soghigian
Copy link
Collaborator

Bryce-Soghigian commented Jan 9, 2024

networkinterfaceapi.go fake BeginDelete incorrectly returns a nil poller

this is addressed in my pr for networkinterface delete

@rakechill
Copy link
Contributor Author

rakechill commented Jan 9, 2024

@rakechill there is more than a single test using normal go tests.

./pkg/auth/cred_test.go ./pkg/auth/config_test.go ./pkg/providers/imagefamily/bootstrap/aksbootstrap_test.go ./pkg/fake/skus_test.go ./pkg/fake/azureresourcegraphapi_test.go ./pkg/utils/gpu_test.go

Are all the go tests. IMO sometimes its nice to write a very simple go test rather than setting up all the ginkgo shenanigans and they are better for fine grained unit tests vs the coarse unit tests we write through the fakes framework.

@Bryce-Soghigian Ack. Maybe a better solution is to call out the different sets of testing we do instead. That could go somewhere on the contributing.md/website. I've written something up in my notes in the meantime. Happy to port over/create a PR as desired.

@rakechill rakechill marked this pull request as ready for review January 10, 2024 02:10
@rakechill rakechill merged commit 8d9211c into main Jan 10, 2024
7 checks passed
@rakechill rakechill deleted the rakechill/refactors branch January 10, 2024 18:54
@tallaxes tallaxes added the area/code-organization Issues or PRs related to code organization label Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/code-organization Issues or PRs related to code organization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants