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

Accept urls and file paths for porter run #187

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

youreddy
Copy link

Resolves #174

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Everything is great, just one unanticipated problem with bundle dependencies that we need to figure out a solution for first.

if err != nil {
return nil, errors.Wrapf(err, "could not parse manifest yaml in %q", path)
}
m.path = path
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we will need to think a bit more about how to handle this variable right here.

The path variable holds the location of the manifest on the local filesystem. Porter uses it to resolve references to dependencies. For example, if the bundle depended upon another bundle named "mysql", one place Porter would look for the mysql bundle would be in the same directory as the bundle on disk, in a directory named "bundles"

- wordpress/
--- bundles/
---- porter.yaml
-- porter.yaml

Once we have registries, and porter uses the local store, this won't be a problem. But until then, we should at least set this to a value that doesn't cause an error.

Perhaps we can set it to "", and then add a check in config.go (GetBundleDir) to skip looking for the local bundle dir when it's empty?

What do you think?

Copy link
Author

@youreddy youreddy Feb 27, 2019

Choose a reason for hiding this comment

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

@carolynvs just revisited the code and saw what you're describing. We could continue setting path to the url and have GetBundleDir skip the lookup if path resembles a url. The benefit of doing that might be a little bit more clarity on why the skip is happening. The snippet of code could look something like:
if strings.HasPrefix(localDir, "http"){ ... }

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Good idea, I'm fine with that solution. 👍

Copy link
Author

Choose a reason for hiding this comment

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

Awesome! Just updated the PR branch.

@carolynvs
Copy link
Member

Looks good, thank you for this improvement! It will make it much easier for people to try out examples.

@carolynvs carolynvs merged commit e61b9d3 into getporter:master Feb 28, 2019
@ghost ghost removed the review label Feb 28, 2019
@youreddy youreddy deleted the issue-#174 branch February 28, 2019 17:11
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.

2 participants