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

Refactor claims plugin to be a storage plugin #859

Merged
merged 14 commits into from
Jan 31, 2020

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented Jan 17, 2020

What does this change

Instead of having a credential plugin, and a claims plugin, etc this refactors the plugin POC so that there is a single storage plugin with the intent of being able to offload the entire porter home directory contents (except the config file) to the plugins.

For example everything that we currently store today on the filesystem could be stored in blob storage or a database. For this PR that means credential sets and claims.

This replies upon a unmerged cnab-go branch with changes to the crud.Store interface and other changes to support this.

I have also stubbed in a new plugin for secrets, though it isn't completely plumbed through and will be completed in a separate PR. At the moment all secrets, for example the values that we resolve
credentials from but could eventually be parameter values too among other things, are coming from a host plugin embedded in porter.

What issue does it fix

#866

Notes for the reviewer

  • There will be a follow-on PR to support secret plugins more. For now it's a stub.
  • Documentation of plugins will come once the framework is stable and ready for people the use.

Checklist

  • Unit Tests
  • Documentation
    • Documentation Not Impacted

@carolynvs carolynvs force-pushed the credential-plugin branch 2 times, most recently from 8ac6766 to 45181c0 Compare January 23, 2020 16:21
@carolynvs carolynvs changed the title WIP: Stare not into the abyss, for it will stare into you Refactor claims plugin to be a storage plugin Jan 23, 2020
Instead of having a credential plugin, and a claims plugin, etc this
refactors the plugin POC so that there is a single storage plugin with
the intent of being able to offload the entire porter home directory
contents (except the config file) to the plugins.

For example everything that we currently store today on the filesystem
could be stored in blob storage or a database.

This replies upon a unmerged branch with changes to the crud.Store
interface and other changes to support this.

I have also stubbed in a new plugin for secrets, though it isn't
completely plumbed through and will be completed in a separate PR. At
the moment all secrets, for example the values that we resolve
credentials from but could eventually be parameter values too among
other things, are coming from a host plugin embedded in porter.
@carolynvs
Copy link
Member Author

It all works. I need to catch up on tests for net new classes and write a bit more doc strings to help explain what's going on.

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Impressive work on such a large and complex endeavor!

All looking good so far to me. Mostly just a few clarifications at this point...

pkg/cnab/provider/action_test.go Outdated Show resolved Hide resolved
pkg/cnab/provider/action_test.go Outdated Show resolved Hide resolved
pkg/config/datastore.go Outdated Show resolved Hide resolved
pkg/credentials/credentialStorage.go Outdated Show resolved Hide resolved
pkg/credentials/credentialStorage.go Show resolved Hide resolved
pkg/credentials/migrateCredentialsWrapper.go Outdated Show resolved Hide resolved
pkg/porter/credentials.go Outdated Show resolved Hide resolved
pkg/secrets/host/store.go Outdated Show resolved Hide resolved
pkg/secrets/pluginstore/store.go Outdated Show resolved Hide resolved
pkg/secrets/pluginstore/store.go Outdated Show resolved Hide resolved
Use the stame instance of the storage pluginStore between the claims
storage and credentials storage.
* Loading from file
* Ensure our mapstructure tags are correct
* Switch to default-storage-plugin for consistency
@carolynvs carolynvs marked this pull request as ready for review January 30, 2020 14:14
@carolynvs
Copy link
Member Author

I have submitted upstream PRs for everything that was in that branch of cnab-go, added tests for all the net new structs and added go docs.

I officially declare this turd polished! 🌟

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

LGTM!

Only non-blocking comments/suggestions.

pkg/config/datastore/loader_test.go Show resolved Hide resolved
pkg/config/datastore_test.go Show resolved Hide resolved
pkg/credentials/credentialStorage.go Show resolved Hide resolved
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

🎉

@carolynvs carolynvs merged commit f921863 into getporter:master Jan 31, 2020
@carolynvs carolynvs deleted the credential-plugin branch January 31, 2020 17:35
This was referenced Feb 12, 2020
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.

3 participants