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

feat(templates): add storage.template; add Makefile targets #6

Merged
merged 2 commits into from
Jan 3, 2019

Conversation

vdice
Copy link
Member

@vdice vdice commented Jan 2, 2019

Open to removing/modifying Makefile additions; just found 'em helpful for development.

@vdice vdice changed the title feat(templates): add storage.template; add build-templates Makefile target feat(templates): add storage.template; add Makefile targets Jan 2, 2019
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.

I love more makefile targets, bring 'em! 👍

I can't really review the template though, so maybe wait on Jeremy for that?

Makefile Outdated
$(BINDIR)/$(MIXIN)$(FILE_EXT) version

test-unit: build
go test ./...

HAS_JSONPP := $(shell command -v jsonpp)
Copy link
Member

Choose a reason for hiding this comment

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

hehe you said pp

@carolynvs
Copy link
Member

oops! You can make a new script step in the azure-pipelines.yml file to install dependencies, like we do here

https://github.com/deislabs/porter/blob/a5ae176d0df8fc4a0bb66dd4ec177b4ec88d4cdc/azure-pipelines.yml#L43-L48

and put it before the unit test step.

That will make the tests pass again.

@vdice
Copy link
Member Author

vdice commented Jan 2, 2019

@carolynvs Thank you! Not sure if I got the script right; will check build and tune if needed. And/or, do you think it'd be better to split build/test dependency fetching into a separate bootstrap/other target?

@vdice vdice force-pushed the feat/azure-storage-template branch 2 times, most recently from 295ca90 to b43b443 Compare January 2, 2019 22:31
@@ -23,6 +23,12 @@ steps:
echo '##vso[task.prependpath]$(GOROOT)/bin'
displayName: 'Set up the Go workspace'

- script: |
go get github.com/gobuffalo/packr/packr
Copy link
Member

Choose a reason for hiding this comment

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

I am ok with either here or in a follow-on PR making another target like make get-deps that does all of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added get-deps. Each target that requires it now calls it, so no real need for a separate bootstrap script here; therefore, I've removed. Let me know if I should add back in to make explicit...

azure-pipelines.yml Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
@vdice vdice force-pushed the feat/azure-storage-template branch 3 times, most recently from e0731f5 to f5da14d Compare January 2, 2019 23:33
@vdice vdice force-pushed the feat/azure-storage-template branch from f5da14d to c2d5d34 Compare January 2, 2019 23:35
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.

Build is green, works on my machine

@carolynvs carolynvs merged commit a068271 into getporter:master Jan 3, 2019
@vdice vdice deleted the feat/azure-storage-template branch January 3, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants