Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Snap Strategy #306
Add Snap Strategy #306
Changes from 3 commits
7d9f44e
021a62e
1b4a250
b06e56e
7ce2ce1
855a963
a58b70c
5fe7a2c
a607856
88f7f09
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
suggestion:
I kind of prefer code like this:
install
functionSo the (dis)advantage are:
Chance to use snap resource in the futureHWToolHelper.install
function. Other design won't be broken.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.
Different snaps will have different channels to install from. I don't see how passing a single channel on HWToolHelper can solve this. I think it would be very strange to have one charm config for channel to rule on all snaps channels.
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.
while I agree that we should remove the constructor for consistency with the other strategies, I don't quite get why it would be preferable to move complexity to the install function, as that requires the caller to have deeper knowledge about how each strategy works and adds extra conditionals to
HWToolHelper.install
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.
We need a protocol to pass all the required material, resources, configuration, and hardware detect result, for the strategies.
So
install
function is the protocol right now. If we break this rule then the ISP(interface-segregation principles) will be broken then you have to find a tricky/hacky way to import those material into the strategies.(I am open to any way to refactor the protocol we have now, but it's a refactor so it shouldn't happen on this PR)
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's a pseudocode code, so pass the configuration you want.
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.
nit
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.
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.
I imagine this is a leftover from manual tests
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.
nit: needs updating