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
Add framework for remote claims storage plugins #699
Conversation
887de64
to
39d3887
Compare
/azp run |
Pull request contains merge conflicts. |
Oops rebasing... |
For now this just returns a filesystem claims store but later we can use this to return stores that are backed by plugins.
39d3887
to
b1fbe99
Compare
Oops, I need to do more work to make the integration tests work when they call porter for internal plugins. BRB rethinking my life choices 💨 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this one! Sounds like there may be some further changes per #699 (comment) but all is looking good to me.
|
||
var _ plugin.Plugin = &Plugin{} | ||
|
||
// P |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P
relude to a full description/comment? 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
"github.com/deislabs/porter/pkg/plugins" | ||
"github.com/hashicorp/go-hclog" | ||
"github.com/hashicorp/go-plugin" | ||
_ "github.com/hashicorp/go-plugin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: What does this import do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, this was when I was first working with the lib, it's not needed anymore. I'll remove it.
if err != nil { | ||
return nil, nil, errors.Wrap(err, "could not determine the plugins directory path") | ||
} | ||
pluginsDir := filepath.Join(home, "plugins") // TODO: move to config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder for this TODO
:)
p.Builder = NewTestBuildProvider() | ||
p.InstanceStorage = instancestorage.NewTestInstanceStorageProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this line need to come before line 42 above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does! I'm fixing it now locally to get the integration tests working
Sorry about the low quality PR. I should have waited on asking for feedback until I had figured out why the integration test for dependencies was failing first. |
@carolynvs Your PRs are the highest-quality around! I always learn a ton from them, both code-related and how to make better PRs myself :) |
What does this change
For now this just returns a filesystem claims store but later we can use this to return stores that are backed by plugins.
What issue does it fix
Closes # (issue)
Notes for the reviewer
Put any questions or notes for the reviewer here.
Checklist