laskoviymishka commented on code in PR #2018:
URL: https://github.com/apache/iceberg-go/pull/2018#discussion_r4045066724
##########
io/gocloud/azure/azure_test.go:
##########
@@ -23,39 +23,61 @@ import (
"net/url"
"testing"
+ "github.com/Azure/azure-sdk-for-go/sdk/azcore"
+ "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake"
+ "github.com/Azure/azure-sdk-for-go/sdk/azidentity"
+ icebergio "github.com/apache/iceberg-go/io"
"github.com/apache/iceberg-go/io/gocloud/blobfs"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gocloud.dev/blob"
"gocloud.dev/blob/memblob"
)
+func TestProductionAzureCredentialFactories(t *testing.T) {
+ factories := productionAzureCredentialFactories()
+
+ t.Run("default credential", func(t *testing.T) {
+ // Avoid inheriting credential selection or loading credentials
from the environment.
+ t.Setenv("AZURE_TOKEN_CREDENTIALS", "dev")
+
+ credential, err := factories.newDefaultCredential(nil)
+ require.NoError(t, err)
+ assert.IsType(t, &azidentity.DefaultAzureCredential{},
credential)
+ })
+
+ t.Run("managed identity credential", func(t *testing.T) {
+ credential, err := factories.newManagedIdentity(nil)
+ require.NoError(t, err)
+ assert.IsType(t, &azidentity.ManagedIdentityCredential{},
credential)
+ })
+}
+
func TestCreateAzureBucketDefaultCredentialCalled(t *testing.T) {
ctx := context.Background()
parsedURL, err :=
url.Parse("abfs://[email protected]/path")
assert.NoError(t, err)
- props := map[string]string{}
+ defaultCredentialCalled := false
+ credentialFactories := azureCredentialFactories{
+ newDefaultCredential:
func(*azidentity.DefaultAzureCredentialOptions) (azcore.TokenCredential, error)
{
+ defaultCredentialCalled = true
- bucket, err := createAzureBucket(ctx, parsedURL, props)
- if err != nil {
- // If bucket creation fails, it should be with a
DefaultAzureCredential error
- assert.Contains(t, err.Error(), "DefaultAzureCredential",
- "Expected DefaultAzureCredential error but got: %v",
err)
- t.Logf("DefaultAzureCredential path was taken and failed at
creation: %v", err)
+ return &fake.TokenCredential{}, nil
+ },
+ newManagedIdentity:
func(*azidentity.ManagedIdentityCredentialOptions) (azcore.TokenCredential,
error) {
+ t.Fatal("managed identity credential factory should not
be called")
- return
+ return nil, nil
Review Comment:
The `t.Fatal` covers us today since both closures run on the test goroutine,
but `return nil, nil` is a fragile sentinel: a nil credential with a nil error.
If a future refactor ever calls these from a background goroutine, `t.Fatal`'s
`Goexit` only kills that goroutine, the return actually executes, and the
caller gets a nil credential it thinks is valid.
I'd return an error here instead:
```go
t.Errorf("managed identity credential factory should not be called")
return nil, fmt.Errorf("unexpected call to managed identity credential
factory")
```
Same pattern applies to the default-credential closure over in the
managed-identity test (line 111).
##########
io/gocloud/azure/azure_test.go:
##########
@@ -23,39 +23,61 @@ import (
"net/url"
"testing"
+ "github.com/Azure/azure-sdk-for-go/sdk/azcore"
+ "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake"
+ "github.com/Azure/azure-sdk-for-go/sdk/azidentity"
+ icebergio "github.com/apache/iceberg-go/io"
"github.com/apache/iceberg-go/io/gocloud/blobfs"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gocloud.dev/blob"
"gocloud.dev/blob/memblob"
)
+func TestProductionAzureCredentialFactories(t *testing.T) {
+ factories := productionAzureCredentialFactories()
+
+ t.Run("default credential", func(t *testing.T) {
+ // Avoid inheriting credential selection or loading credentials
from the environment.
+ t.Setenv("AZURE_TOKEN_CREDENTIALS", "dev")
Review Comment:
I'd drop this test, or narrow it right down.
`AZURE_TOKEN_CREDENTIALS` isn't something the Go `azidentity` SDK reads
(it's a Java/.NET knob), so this `Setenv` is a no-op and the comment above it
is misleading. And `NewDefaultAzureCredential` always returns
`*DefaultAzureCredential` on success, so the `IsType` assertion is guaranteed
by the signature; it can't fail. The routing is already proven by the two
`...CredentialCalled` tests below, so this one only buys the appearance of
coverage.
If you want to keep a wiring check, I'd assert the factory fields are
non-nil and skip constructing real credentials. wdyt?
##########
io/gocloud/azure/azure_test.go:
##########
@@ -81,20 +103,28 @@ func
TestCreateAzureBucketManagedIdentityCredentialCalled(t *testing.T) {
parsedURL, err :=
url.Parse("abfs://[email protected]/path")
assert.NoError(t, err)
- // NewManagedIdentityCredential never fails at construction, so bucket
creation always succeeds.
- bucket, err := createAzureBucket(ctx, parsedURL, map[string]string{
- "adls.auth.managed-identity.enabled": "true",
- })
- assert.NoError(t, err)
- assert.NotNil(t, bucket)
+ managedIdentityCalled := false
+ credentialFactories := azureCredentialFactories{
+ newDefaultCredential:
func(*azidentity.DefaultAzureCredentialOptions) (azcore.TokenCredential, error)
{
+ t.Fatal("default credential factory should not be
called")
+
+ return nil, nil
+ },
+ newManagedIdentity: func(opts
*azidentity.ManagedIdentityCredentialOptions) (azcore.TokenCredential, error) {
+ managedIdentityCalled = true
+ require.Nil(t, opts)
Review Comment:
`require.Nil(t, opts)` reaches into the production function's local state.
If someone ever defaults `miOpts` to `&ManagedIdentityCredentialOptions{}` it'd
break this test for no behavioral reason, and `managedIdentityCalled = true`
already proves the path was taken.
While we're here: the client-ID branch
(`ManagedIdentityCredentialOptions{ID: ...}`) is never exercised. If you added
a second sub-case that passes `icebergio.ADLSClientID` and asserted `opts.ID ==
azidentity.ClientID("...")`, you'd cover that branch and turn this assertion
into something meaningful instead of dropping it. wdyt?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
