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

feat: add support for updating the authentication information #82

Merged
merged 19 commits into from
Aug 19, 2024

Conversation

colesnodgrass
Copy link
Member

@colesnodgrass colesnodgrass commented Aug 15, 2024

@colesnodgrass colesnodgrass marked this pull request as ready for review August 16, 2024 23:14
Copy link
Collaborator

@perangel perangel left a comment

Choose a reason for hiding this comment

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

This looks good to me. Left a few suggestions, nothing blocking

internal/cmd/local/airbyte/api.go Outdated Show resolved Hide resolved
}
a.mu.Lock()
defer a.mu.Unlock()
// check again if there is now a token
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not following the reason for this check-lock-check-again behavior? Is this intended or leftover from some refactor?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's intended. If two callers to fetchToken come in at the same time, the first would get the lock, create the token, and release the lock, the second would then get the lock, create a new token, and release the lock. This prevents the second caller from recreating the token when the first caller already succeeded.

In the current configuration this could be replaced by a sync.Once to prevent this, but tokens don't last forever and I'm unsure when a new one will need to be fetched. So the token itself isn't immutable.

restartedAtValue := restartedAt.Format(time.RFC3339)

// similar to how kubectl rollout restart works, patch in a restartedAt annotation.
rawPatch := map[string]any{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Y
   E
      S
         S
            S
            !
          ! 
       !
    !
!  

internal/cmd/local/k8s/client.go Outdated Show resolved Hide resolved
}

// check every 10 seconds for up to 5 minutes to see if the pods have been restarted successfully
err = wait.PollUntilContextTimeout(ctx, 5*time.Second, block, true, deploymentPods)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment does not match the invocation (interval is 5 not 10). Also (nit): there may be a clearer than for this arg than block

if err != nil {
return err
}

pterm.Success.Println(fmt.Sprintf("Getting your credentials: %s", secret.Name))
pterm.Info.Println(fmt.Sprintf("{\n \"password\": \"%s\",\n \"client-id\": \"%s\",\n \"client-secret\": \"%s\"\n}", secret.Data["instance-admin-password"], secret.Data["instance-admin-client-id"], secret.Data["instance-admin-client-secret"]))
clientId := string(secret.Data[secretClientID])
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT (iirc): I believe secret has a .StringData property that would mean you don't need the cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the docs

	// stringData allows specifying non-binary secret data in string form.
	// It is provided as a write-only input field for convenience.
	// All keys and values are merged into the data field on write, overwriting any existing values.
	// The stringData field is never output when reading from the API.

I don't think it is actually returned from the get/read call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, makes sense I think the context where I used it was in some tests so I may have actually written to it myself

@colesnodgrass colesnodgrass merged commit d472e0f into main Aug 19, 2024
2 checks passed
@colesnodgrass colesnodgrass deleted the cole/creds-flags branch August 19, 2024 22:11
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.

2 participants