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

Prep Dockerfile for building with ART tooling. #38

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

dgoodwin
Copy link

@dgoodwin dgoodwin commented Sep 7, 2018

Move Dockerfile to project root to accomodate tooling, needs to have
source in build context.

Make sure GOPATH is set, it may not be in some build images that get
auto-replaced in for the FROM.

Workaround a bug in some versions of imagebuilder where WORKDIR will not
create the directory if it doesn't exist.

We can't yet test this but I spoke with lmeyer and he spotted a couple things that might be needed, so submitting a PR for those. Let me know if anything else needs to be updated, I may have missed other routes to the Dockerfile I'm not aware of.

CC @enxebre @paulfantom

Move Dockerfile to project root to accomodate tooling, needs to have
source in build context.

Make sure GOPATH is set, it may not be in some build images that get
auto-replaced in for the FROM.

Workaround a bug in some versions of imagebuilder where WORKDIR will not
create the directory if it doesn't exist.
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 7, 2018
@paulfantom
Copy link

@dgoodwin @enxebre what about cluster-controller Dockerfile?

@dgoodwin
Copy link
Author

dgoodwin commented Sep 7, 2018

Is it used? As far as I could tell it isn't.

If so we may have an issue, there may be some problems doing multiple images from one repo. We could comebine it into the other image though.

@enxebre
Copy link
Member

enxebre commented Sep 10, 2018

We don't need cluster controller.
We need:
cluster-apiserver
Controller manager
Machine controller

@dgoodwin
Copy link
Author

And you won't need cluster-apiserver for long either when they go to CRDs.

@dgoodwin
Copy link
Author

/retest

@paulfantom
Copy link

About failing tests:

@dgoodwin
Copy link
Author

Aha, thanks @paulfantom . Do we just get the release PR merged first and then this one or do we need to force merge both due to the deps?

@paulfantom
Copy link

We can merge release one first and then this one. I have also started fixing linter stuff in #39.

@dgoodwin
Copy link
Author

/retest

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 10, 2018

@dgoodwin: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/go-vet bc6e185 link /test go-vet
ci/prow/golint bc6e185 link /test golint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@paulfantom
Copy link

Image testing is OK. Everything LGTM.

@paulfantom
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgoodwin, paulfantom

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2018
@paulfantom paulfantom merged commit 7957e91 into openshift:master Sep 10, 2018
@dgoodwin dgoodwin deleted the art-dockerfile-updates branch September 10, 2018 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants