Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

Allow branches or tags to be specified as a fragment on the build pack url #51

Closed
wants to merge 5 commits into from

Conversation

fraenkel
Copy link
Contributor

This modification does not support SHAs.
With the use of --depth 1, certain tags are not supported.
If the branch/tag provided does not exist, git will pull from HEAD

@nebhale
Copy link
Contributor

nebhale commented Aug 21, 2013

/cc @rmorgan @glyn

@nebhale
Copy link
Contributor

nebhale commented Aug 21, 2013

@fraenkel First a suggestion that I know the Runtime team will make. Please squash all your commits into a single commit and then force push it back to the same branch name. They'll want a single commit that is the net of all the changes in this branch. Using the force push will re-use this pull request for that new squashed commit.

Second, can you please explain what you mean by the phrase: "With the use of --depth 1, certain tags are not supported." Under what circumstance won't a tag work?

@fraenkel
Copy link
Contributor Author

As far as commits are concerned, the pull request can contain as many commits as you need but must be separate from any other commits like rebases. I just went through this exercise on another pull request.

As far as tags are concerned, only annotated tags can't be cloned properly. If you have a lightweight tag, like you do in the java-buildpack, those cannot be cloned with the -b option.

@nebhale
Copy link
Contributor

nebhale commented Aug 21, 2013

@fraenkel, you used the negative twice in that last sentence. Did you mean that annotated tags cannot, but lightweight tags can?

@fraenkel
Copy link
Contributor Author

Sorry... Annotated tags can be cloned but lightweight cannot.

@ryantang
Copy link
Contributor

@fraenkel - This PR is really doing two different things - one is cloning with the depth 1 flag and the second is only cloning a branch. We went ahead and added the depth 1 flag in this commit: bbb5ea1 since it is something we should be doing anyway.

For cloning just a branch (which really means cloning just a ref), we would like to avoid using a non-standard git URI - the fragment approach is not something that git itself supports. @vito's comment on PR #24 (comment) lays out the proper direction that we would like to take for something like this since there are quite a few moving parts. That being said, we do have a story to implement this, however it's in our icebox currently meaning we likely won't be getting to it for some time.

We are closing this PR since although this is a feature we would like to see, this implementation isn't quite the way we would like to see it done.

@jfoley / @ryantang

@ryantang ryantang closed this Aug 21, 2013
@nebhale
Copy link
Contributor

nebhale commented Aug 21, 2013

@ryantang Does this mean that the decision has been made to break Heroku compatibility on this feature?

@mkocher
Copy link
Contributor

mkocher commented Aug 22, 2013

I need to dig into this a bit today and make sure we've all got the same idea about the right approach. Lets leave this open while it's still under discussion.

@mkocher mkocher reopened this Aug 22, 2013
@mkocher
Copy link
Contributor

mkocher commented Aug 22, 2013

Also - We're still figuring out when to leave pull requests and/or issues open. Please bear with us and feel free to ask to have things reopened if we close anything prematurely.

@nebhale
Copy link
Contributor

nebhale commented Aug 23, 2013

@mkocher Thanks for the info (and the re-open).

@xoebus
Copy link

xoebus commented Aug 28, 2013

@nebhale — Interesting. Thanks for the update. We were aware that the git://repo.git#master wasn't a git or GitHub syntax, but we'll take another look into this as it's a Heroku "standard".

@ryantang / @xoebus

/cc @mkocher @MarkKropf

@xoebus
Copy link

xoebus commented Aug 30, 2013

@jandubois has brought up an interesting point in another pull request to add this functionality: does the shallow clone here stop you from being able to check out any reference?

@jandubois
Copy link

@xoebus - even if the shallow copy did copy the commit you want to check out, the git clone --branch only takes a branch name, not an arbitrary revision:

$ git clone --branch ceaa830 https://github.com/cloudfoundry/heroku-buildpack-nodejs
Cloning into 'heroku-buildpack-nodejs'...
fatal: Remote branch ceaa830 not found in upstream origin

That's why I didn't use git clone --branch in my PR, but ran an additional git checkout REVISION if the URL contained a revision fragment.

@fraenkel
Copy link
Contributor Author

I will update my pull request to deal with SHAs and lightweight tags as a fallback.
For the 99% case, the current code will suffice. If we detect what was pulled doesn't match the tag/SHA requested, it will unshallow the clone and then do the checkout.

@mark-rushakoff
Copy link
Contributor

There's a lot of discussion and context here. @MarkKropf what would you like to see happen with this pull request?

@mark-rushakoff / @matthewmcnew

@MarkKropf
Copy link

@mark-rushakoff There is already a story referencing this pull request in the backlog and prioritized. We are going to follow the heroku model here and include it in the uri. This PR and others are referenced in our story.

@christo4ferris
Copy link

@MarkKropf is this the story?

buildpack git url should respect a SHA of the commit/tag so the buildpack can be pinned to a particular version

It us unstarted, do you have an ETA on which sprint it might be expected to land? Is there anything we can do to help?

thx

@christo4ferris
Copy link

... it is* unstarted, ...

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

9 participants