Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

AWS polish #117

Merged
merged 36 commits into from
Aug 28, 2019
Merged

AWS polish #117

merged 36 commits into from
Aug 28, 2019

Conversation

jpcoenen
Copy link
Member

Final changes to make the new client usable with the CLI.

By not supplying the Verifier and Encrypter at creation, it is now possible to embed a Key in a KeyCreator. This has the added benefit that a KeyCreator also implements the Provider interface. This is needed for creating a user and directly creating an AccountKey afterwards.
io.Reader suggests that it allows streaming. However, we do not want streaming when reading these small files. It adds a lot of unnecessary complexity. Therefore we chose to use a custom Reader interface that does not imply any streaming functionality.
With the addition of ConfigDir to Client, it is now possible to determine the default credential completely in the client. That way the credentials package does not have to know about the current configuration directory and can remain stateless.
@jpcoenen jpcoenen requested a review from mackenbach August 27, 2019 12:04
pkg/secrethub/configdir/dir.go Outdated Show resolved Hide resolved
pkg/secrethub/configdir/dir.go Outdated Show resolved Hide resolved
pkg/secrethub/configdir/dir.go Outdated Show resolved Hide resolved
jpcoenen and others added 2 commits August 27, 2019 14:08
Not all usages of string were replaced by []byte yet.
pkg/secrethub/client.go Outdated Show resolved Hide resolved
pkg/secrethub/configdir/dir.go Outdated Show resolved Hide resolved
Again, this name suffers from the complexity that it isn't the
'default' per se: it also allows configuration from the environment.

It is more the default way to get a credential than that it is the
default credential itself. Small but important difference in meaning.
We'll leave it as is for now and may revisit it later.
Copy link
Member

@mackenbach mackenbach left a comment

Choose a reason for hiding this comment

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

Nice work. Looks pretty clean. I've made a few comments and implemented most of the ones concerning godocs.

pkg/secrethub/client_options.go Show resolved Hide resolved
pkg/secrethub/configdir/dir.go Show resolved Hide resolved
pkg/secrethub/configdir/dir.go Outdated Show resolved Hide resolved
pkg/secrethub/configdir/dir.go Outdated Show resolved Hide resolved
pkg/secrethub/credentials/creators.go Outdated Show resolved Hide resolved
pkg/secrethub/credentials/providers.go Outdated Show resolved Hide resolved
pkg/secrethub/credentials/readers.go Show resolved Hide resolved
pkg/secrethub/client.go Outdated Show resolved Hide resolved
pkg/secrethub/client.go Show resolved Hide resolved
pkg/secrethub/credentials/encoding.go Outdated Show resolved Hide resolved
@SimonBarendse SimonBarendse marked this pull request as ready for review August 28, 2019 08:13
Copy link
Member

@mackenbach mackenbach left a comment

Choose a reason for hiding this comment

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

1 thing I noticed, otherwise nice changes 👍

pkg/secrethub/credentials/key.go Outdated Show resolved Hide resolved
jpcoenen and others added 6 commits August 28, 2019 10:39
There is no need to change this. Therefore making it private and only
accessible through Path() makes more sense.
Fingerprint and the bytes of a verifier are always used together and both functions need each other. Therefore it makes sense to have one function that retrieves both. This makes using it a lot easier.
This was necessary because of the previous commit Export() was used twice. Encode() makes more sense anyway because that is exactly what this method does.
@jpcoenen jpcoenen merged commit 09c4a27 into feature/aws-integration-merge Aug 28, 2019
@jpcoenen jpcoenen deleted the feature/aws-polish branch August 28, 2019 09:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants