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

Add Builder provider interface #6626

Merged
merged 1 commit into from Jun 6, 2019

Conversation

@chefsalim
Copy link
Member

commented Jun 6, 2019

This change abstracts out the Builder API into a provider model, in readiness for adding a new API provider in the builder-api-client crate. Functionality wise, it is a no-op. There's a bit of change that spills out from the builder-api-client due to managing the DisplayProgress trait in the provider, but its fairly minor.

Signed-off-by: Salim Alam salam@chef.io

tenor-96156703

@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 6, 2019

Hello chefsalim! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@eeyun

eeyun approved these changes Jun 6, 2019

@chefsalim chefsalim force-pushed the SA/artifactory-client branch from a9e58a5 to c18a1cf Jun 6, 2019

@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Can you provide pointers to where there is code that has changed? It's kind of hard to see what to review exactly.

@chefsalim

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@baumanj The bulk of the API code is just moved over from lib.rs to builder.rs. The changes there - if you look at the deleted code in lib.rs, there are some APIs that had generic params - those were modified to be non-generic when moved to builder.rs. There's only a few of those. Other than that, some things that were unused were removed - eg, fetch_origin_secret_key (which is not used by hab - and builder has its own mini-API so it doesn't use this interface).

From the caller's perspective, the main change is the return type from Client::new, and the type of UIWriter::progress.

}

impl BuilderAPIProvider for BuilderAPIClient {
type Progress = Box<DisplayProgress>;

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

Since the introduction of the dyn keyword, this usage of trait objects is discouraged and will probably eventually become a warning. I also find it clearer since it makes it explicit that DisplayProgress is a trait object, not a struct/enum

Suggested change
type Progress = Box<DisplayProgress>;
type Progress = Box<dyn DisplayProgress>;

This comment has been minimized.

Copy link
@chefsalim

chefsalim Jun 6, 2019

Author Member

Ah yes.. will update to the new syntax - thanks.

.to_string());
}
pub struct Client;
pub type BoxedClient = Box<BuilderAPIProvider<Progress = Box<DisplayProgress>>>;

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member
Suggested change
pub type BoxedClient = Box<BuilderAPIProvider<Progress = Box<DisplayProgress>>>;
pub type BoxedClient = Box<BuilderAPIProvider<Progress = Box<dyn DisplayProgress>>>;
@@ -293,7 +293,7 @@ pub trait UIWriter {
/// Messages sent to the error IO stream will be formatted for a terminal if true.
fn is_err_a_terminal(&self) -> bool;
/// Returns a progress bar widget implementation for writing operation's progress to.
fn progress(&self) -> Option<Self::ProgressBar>;
fn progress(&self) -> Option<Box<DisplayProgress>>;

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member
Suggested change
fn progress(&self) -> Option<Box<DisplayProgress>>;
fn progress(&self) -> Option<Box<dyn DisplayProgress>>;
@@ -487,9 +487,9 @@ impl UIWriter for UI {

fn is_err_a_terminal(&self) -> bool { self.shell.err.is_a_terminal() }

fn progress(&self) -> Option<Self::ProgressBar> {
fn progress(&self) -> Option<Box<DisplayProgress>> {

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member
Suggested change
fn progress(&self) -> Option<Box<DisplayProgress>> {
fn progress(&self) -> Option<Box<dyn DisplayProgress>> {
@@ -126,9 +126,9 @@ impl UIWriter for CtlRequest {

fn is_err_a_terminal(&self) -> bool { true }

fn progress(&self) -> Option<Self::ProgressBar> {
fn progress(&self) -> Option<Box<DisplayProgress>> {

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member
Suggested change
fn progress(&self) -> Option<Box<DisplayProgress>> {
fn progress(&self) -> Option<Box<dyn DisplayProgress>> {

@chefsalim chefsalim force-pushed the SA/artifactory-client branch 2 times, most recently from ea23831 to 0584ab4 Jun 6, 2019

@chefsalim chefsalim requested a review from baumanj Jun 6, 2019

@baumanj

baumanj approved these changes Jun 6, 2019

Copy link
Member

left a comment

Looks like one place got missed. Fix that up and this looks good to me.

Add Builder provider interface
Signed-off-by: Salim Alam <salam@chef.io>

@chefsalim chefsalim force-pushed the SA/artifactory-client branch from 0584ab4 to a52bccf Jun 6, 2019

@chefsalim chefsalim merged commit 8d0386f into master Jun 6, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2209 passed (30 minutes, 21 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2450 passed (33 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@chefsalim chefsalim deleted the SA/artifactory-client branch Jun 6, 2019

chef-ci added a commit that referenced this pull request Jun 6, 2019

Update CHANGELOG.md with details from pull request #6626
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.