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

Add support for building from a subdirectory#245

Merged
google-prow-robot merged 4 commits intoknative:masterfrom
evankanderson:subdir
Jul 21, 2018
Merged

Add support for building from a subdirectory#245
google-prow-robot merged 4 commits intoknative:masterfrom
evankanderson:subdir

Conversation

@evankanderson
Copy link
Copy Markdown
Member

The major change is to have all stages after the "source fetch" possibly pivot into a subdir of the workspace volume via the subPath option to VolumeMount. Tested by hand, but will add an e2e test.

Also fixes test credential handling on Windows.

Fixes #91

Proposed Changes

  • Adds a subDir option to any source type specifying that only a subdirectory of the source should be built.
  • Fixes SSH credentials tests when run on windows (because it made my tests fail).

Release Note

* Adds a new field, `sshDir`, to the `Source` specification to allow
  building just one subdirectory of a repo. This is useful for samples
  and tests which are part of a larger repo.

for i, step := range append(setupContainers, build.Spec.Steps...) {
step.Env = append(implicitEnvVars, step.Env...)
// TODO(mattmoor): Check that volumeMounts match volumes.
step.VolumeMounts = append(step.VolumeMounts, mounts...)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I moved these so that the Name and WorkingDir changes happen after the copies.

IdentityFile %s/.ssh/id_foo
`, os.Getenv("HOME"))
IdentityFile %s
`, filepath.Join(os.Getenv("HOME"), ".ssh", "id_foo"))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can pull creds_test.go and ssh.go into a separate PR if you want. They were small, so I added them for now.

@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Jul 11, 2018

/test pull-knative-build-go-coverage

3 similar comments
@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Jul 11, 2018

/test pull-knative-build-go-coverage

@krzyzacy
Copy link
Copy Markdown

/test pull-knative-build-go-coverage

@evankanderson
Copy link
Copy Markdown
Member Author

/test pull-knative-build-go-coverage

Comment thread pkg/apis/build/v1alpha1/build_types.go Outdated
Custom *corev1.Container `json:"custom,omitempty"`

// SubDir specifies a subdirectory of the fetched source which should be
// built.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mention here that this makes parent directories inaccessible.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. I also mentioned the possible optimization that files outside the subPath may not be fetched.

Comment thread pkg/apis/build/v1alpha1/build_types.go Outdated

// SubDir specifies a subdirectory of the fetched source which should be
// built.
SubDir string `json:"subDir,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:bikeshed: WDYT about Dir or SubPath?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Chose SubPath for consistency with k8s volumeMount.

Comment thread tests/subdir/0-template.yaml Outdated
# See the License for the specific language governing permissions and
# limitations under the License.
apiVersion: build.knative.dev/v1alpha1
kind: BuildTemplate
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't really need a template to demonstrate the capability, I'd just specify a build with source.subDir directly.

This would also let each test be explicit about what they're doing:

  • subdir-exists.yaml specifies a subdir that exists, and its steps run cat <some file that's only in that subdir>
  • subdir-notexists.yaml specifies a subdir that doesn't exist and expects failure
  • subdir-nofile.yaml specifies a subdir that exists but a file that doesn't exist in that subdir, and so expects failure.

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-knative-build-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/builder/cluster/convert/convert.go 83.6% 84.9% 1.3

@evankanderson
Copy link
Copy Markdown
Member Author

/retest

@evankanderson
Copy link
Copy Markdown
Member Author

Needs /approve and /lgtm still. 😄

Copy link
Copy Markdown
Contributor

@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.

/lgtm
/approve

GCS *GCSSourceSpec `json:"gcs,omitempty"`
Custom *corev1.Container `json:"custom,omitempty"`

// SubPath specifies a pathd within the fetched source which should be
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: path

@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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

@google-prow-robot google-prow-robot merged commit 3e9d2fd into knative:master Jul 21, 2018
@evankanderson evankanderson deleted the subdir branch July 24, 2018 14:09
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
* Add support for building from a subdirectory

Also fixes test credential handling on Windows.

* Add e2e test for subdir option

* Rename `subDir` to `subPath`, address other @imjasonh feedback

* Rename test files to `subpath`
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
* Add support for building from a subdirectory

Also fixes test credential handling on Windows.

* Add e2e test for subdir option

* Rename `subDir` to `subPath`, address other @imjasonh feedback

* Rename test files to `subpath`
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.

git source should have an optional "src_dir" setting

6 participants