Skip to content
This repository has been archived by the owner on Sep 6, 2019. It is now read-only.

Add BuildTemplate for makisu #90

Merged

Conversation

johscheuer
Copy link
Contributor

This PR adds a simple BuildTemplate for makisu (https://github.com/uber/makisu). As soon as knative/build#550 gets merged I would adjust the Secret Volume to be rendered by a Template Parameter rather than a hardcoded string.

Currently the makisu setup doesn't use a global build cache (mentioned here: https://github.com/uber/makisu#using-cache) if you want I can add an example for the global cache too.

Fixes: #77

@knative-prow-robot
Copy link

Hi @johscheuer. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@johscheuer
Copy link
Contributor Author

ping @imjasonh and @mattmoor

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, some comments.

makisu/README.md Outdated Show resolved Hide resolved
(_required_)
* **CONTEXTPATH**: The path to the build context (_default:_
`/workspace`)
* **PUSH_REGISTRY**: The Registry to push the image to (_default:_
Copy link
Member

Choose a reason for hiding this comment

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

How does this work if IMAGE is not a Dockerhub image? Is it just unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need to create a secret for the registry like in the docker example and then reference it with PUSH_REGISTRY, makisu also supports to push to multiple registries (which is currently not implemented in this PR.

makisu/makisu.yaml Outdated Show resolved Hide resolved
makisu/makisu.yaml Outdated Show resolved Hide resolved
@imjasonh
Copy link
Member

imjasonh commented Mar 6, 2019

/lgtm
/approve

@imjasonh
Copy link
Member

imjasonh commented Mar 6, 2019

/lgtm

Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

I modified your README formatting to appease markdownlint so it would unblock merging.

/lgtm

@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ImJasonH, johscheuer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 5fe927b into knative:master Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants