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

[WIP] Support GitSource.Dir#226

Closed
imjasonh wants to merge 1 commit intoknative:masterfrom
imjasonh:srcdir
Closed

[WIP] Support GitSource.Dir#226
imjasonh wants to merge 1 commit intoknative:masterfrom
imjasonh:srcdir

Conversation

@imjasonh
Copy link
Copy Markdown
Contributor

@imjasonh imjasonh commented Jul 6, 2018

Fixes #91

Proposed Changes

  • Add Dir to GitSourceSpec and insert this "srcDir" into steps' workingdirs if specified.
  • Support absolute paths in steps' workingdir, which overrides any srcDir and the default /workspace dir.
  • Specifying a git.dir does not perform a narrow clone and the entire repo's contents at the specified revision are still fetched. This simply modifies the workingdir to operate within the specified dir, if the step's workingdir is unspecified or relative.

This change currently breaks tests because of the lossy conversion when inserting srcDir between /workspace and stepDir -- it's impossible to know how to convert a Build into a Pod and back without knowing the borders should be between srcDir and stepDir. It's possible we'll decide we don't care about this lossy conversion, since the Pod will execute the same regardless, but in that case we'll need to teach tests how to accept this lossiness.

Release Note

Support GitSource.Dir

@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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
Copy link
Copy Markdown

google-prow-robot commented Jul 6, 2018

@imjasonh: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-knative-build-unit-tests 02062d3 link /test pull-knative-build-unit-tests

Full PR test history. Your PR dashboard.

Details

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.

@evankanderson
Copy link
Copy Markdown
Member

Is this still WIP?

I have #245 that I've been working on concurrently; note that I took a different approach and moved all subsequent build steps into the subdir. I also made the subDir option apply to all sources, not just git.

Let me know which way you want to go here -- I'm happy to review this once the "WIP" label is dropped, or you can review #245, which seems to pass in my cluster.

if step.WorkingDir == "" {
step.WorkingDir = "/workspace"
}
step.WorkingDir = workdir(srcDir, step.WorkingDir)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think Git.Dir works here for e.g. the kaniko builder without also updating the path to the Docker image, which seems like a minus over #245.

//
// This must be a relative path. If a step's `workingdir` is specified
// and is an absolute path, this value is ignored for the step's
// execution.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have a good use case for the absolute vs relative path case?

@imjasonh
Copy link
Copy Markdown
Contributor Author

Probably going to go with #245 instead

@imjasonh imjasonh closed this Jul 18, 2018
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.

3 participants