Fix git-init in case `master` is not the default branch #2835

vinamra28 · 2020-06-18T17:04:11Z

Changes

This fix will patch the git-init such that if user doesn't provides revision then instead of taking master as the default branch we are doing the symbolic-link to the HEAD branch. Instead of making the assumption that we should use master when the revision is empty, we are inspecting the remote.

Fixes #2822

Signed-off-by: vinamra28 [email protected]

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

The Git PipelineResource now inspects remote repository to determine the correct default branch to use when no revision has been specified.

/kind bug

tekton-robot · 2020-06-18T17:04:26Z

Hi @vinamra28. Thanks for your PR.

I'm waiting for a tektoncd 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

tekton-robot · 2020-06-18T17:04:34Z

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

ghost · 2020-06-18T17:14:37Z

/ok-to-test

vinamra28 · 2020-06-19T09:41:25Z

@vdemeester could you please take a look at the PR 🙂

vdemeester · 2020-06-19T09:57:38Z

pkg/git/git.go

@@ -65,7 +65,7 @@ func Fetch(logger *zap.SugaredLogger, spec FetchSpec) error {
 	}

 	if spec.Revision == "" {
-		spec.Revision = "master"
+		spec.Revision = "HEAD"


I think we should run git symbolic-ref refs/remotes/origin/HEAD only if spec.Revision is empty and extract the default branch name from it. So something like :

remoteHead, err := run(logger, "", "symbolic-ref", "refs/remotes/origin/HEAD") if err != nil { return err } spec.Revision = strings.ReplaceAll(remoteHead, "refs/remotes/origin/", "") // this would display `master` or `main`, or …

@vdemeester : I tried this out locally on my terminal and also as you suggested but this didn't worked out. On my terminal I got the following error:-

[vinjain@vinjain output]$ git remote show origin * remote origin Fetch URL: https://github.com/spring-projects/spring-petclinic.git Push URL: https://github.com/spring-projects/spring-petclinic.git HEAD branch: main Remote branches: gh-pages new (next fetch will store in remotes/origin) main new (next fetch will store in remotes/origin) wavefront new (next fetch will store in remotes/origin) [vinjain@vinjain output]$ git symbolic-ref refs/remotes/origin/HEAD fatal: ref refs/remotes/origin/HEAD is not a symbolic ref

@vinamra28 you are right, it needs to happend after the fetch 🤔

vinamra28 · 2020-06-20T05:40:56Z

/retest

vdemeester

I think we should have an e2e test that runs against sprint-petclinic or a tektoncd/* controlled repository with a default branch with non master to make sure it works fine 👼

vinamra28 · 2020-06-23T09:03:25Z

@vdemeester added e2e tests as well for both v1beta1 and v1alpha1 for spring-petclinic repository

tekton-robot · 2020-06-23T09:04:44Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	85.4%	84.8%	-0.6

tekton-robot · 2020-06-23T11:02:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	85.4%	84.8%	-0.6

pkg/apis/resource/v1alpha1/git/git_resource_test.go

tekton-robot · 2020-06-23T12:46:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/resource/v1alpha1/git/git_resource.go	85.4%	84.8%	-0.6

vinamra28 · 2020-06-23T18:04:58Z

/test pull-tekton-pipeline-integration-tests

vdemeester

/lgtm

vdemeester · 2020-06-24T07:32:46Z

test/v1alpha1/git_checkout_test.go

+func getGitPipelineResourceSpringPetClinic(revision, refspec, sslverify, httpproxy, httpsproxy, noproxy string) *v1alpha1.PipelineResource {
+	return tb.PipelineResource(gitSourceResourceName, tb.PipelineResourceSpec(
+		v1alpha1.PipelineResourceTypeGit,
+		tb.PipelineResourceSpecParam("Url", "https://github.com/spring-projects/spring-petclinic"),


I think, long term, we should use a project we control (just in case) 😝

actually there was no repo in tektoncd/* which does not have master as the default branch. Earlier we had the v1beta1 branch in tektoncd/catalog as the default branch but now it has been merged to master. That's why I went to spring-petclinic repo for testing 😅

@vinamra28 yep, that's fine for now 😉

Created #2854 to track.

ghost · 2020-06-24T12:49:33Z

I think there are two more things that need to be resolved and then this is ready to merge:

docs/resources.md should include a description of this new behavior. Right now it says:

_If no revision is specified, the resource will default to master`.
We should have Release Notes in the PR description describing the user-visible change. Something like this:

The Git PipelineResource now inspects remote repositories to determine the correct default branch to use when no revision has been specified.

As an aside, we'll need to also update documentation for the git-clone catalog entry when we move to this new version of git-init. I'll create an issue for this.

vinamra28 · 2020-06-24T12:55:03Z

Thank you @sbwsg. Will do this. Thanks for guidance 😄

This fix will patch the `git-init` such that if user doesn't provides revision then instead of taking `master` as the default branch we are doing the `symbolic-link` to the HEAD branch. Also added e2e tests for it. Signed-off-by: vinamra28 <[email protected]>

vdemeester

/lgtm

tekton-robot · 2020-06-24T13:26:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, vdemeester

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:

~~OWNERS~~ [sbwsg,vdemeester]

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

vinamra28 · 2020-06-24T14:06:59Z

/test pull-tekton-pipeline-integration-tests

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 18, 2020

tekton-robot requested review from bobcatfish and imjasonh June 18, 2020 17:04

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2020

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2020

vdemeester requested changes Jun 19, 2020

View reviewed changes

vinamra28 force-pushed the vinamra28/git-res-fix branch 2 times, most recently from fd38d0e to f5ec9c7 Compare June 19, 2020 12:40

vinamra28 force-pushed the vinamra28/git-res-fix branch from f5ec9c7 to 12d3a8c Compare June 20, 2020 07:22

vdemeester reviewed Jun 22, 2020

View reviewed changes

vinamra28 force-pushed the vinamra28/git-res-fix branch from 12d3a8c to 6eff8c4 Compare June 23, 2020 09:01

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 23, 2020

vinamra28 force-pushed the vinamra28/git-res-fix branch 2 times, most recently from 4cc2688 to fde0cfb Compare June 23, 2020 10:59

vinamra28 force-pushed the vinamra28/git-res-fix branch from fde0cfb to 12903d9 Compare June 23, 2020 12:43

ghost reviewed Jun 23, 2020

View reviewed changes

pkg/apis/resource/v1alpha1/git/git_resource_test.go Outdated Show resolved Hide resolved

vinamra28 force-pushed the vinamra28/git-res-fix branch from 12903d9 to c0eb888 Compare June 23, 2020 17:27

vdemeester reviewed Jun 24, 2020

View reviewed changes

tekton-robot assigned vdemeester Jun 24, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020

ghost mentioned this pull request Jun 24, 2020

Update git-clone to latest git-init and document changes in behaviour tektoncd/catalog#390

Closed

vinamra28 force-pushed the vinamra28/git-res-fix branch from c0eb888 to 87c23e1 Compare June 24, 2020 13:06

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020

ghost mentioned this pull request Jun 24, 2020

Modify git-init symbolic ref test to use tektoncd-owned repo or similar #2854

Closed

ghost approved these changes Jun 24, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2020

vdemeester approved these changes Jun 24, 2020

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2020

tekton-robot merged commit 9aff0a0 into tektoncd:master Jun 24, 2020

vinamra28 deleted the vinamra28/git-res-fix branch June 24, 2020 14:56

divyansh42 mentioned this pull request Jul 20, 2020

Update git-clone task and document changes in behaviour tektoncd/catalog#427

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix git-init in case `master` is not the default branch #2835

Fix git-init in case `master` is not the default branch #2835

vinamra28 commented Jun 18, 2020 •

edited

Loading

tekton-robot commented Jun 18, 2020

tekton-robot commented Jun 18, 2020

ghost commented Jun 18, 2020

vinamra28 commented Jun 19, 2020

vdemeester Jun 19, 2020

vinamra28 Jun 19, 2020

vdemeester Jun 19, 2020

vinamra28 commented Jun 20, 2020

vdemeester left a comment

vinamra28 commented Jun 23, 2020

tekton-robot commented Jun 23, 2020

tekton-robot commented Jun 23, 2020

tekton-robot commented Jun 23, 2020

vinamra28 commented Jun 23, 2020

vdemeester left a comment

vdemeester Jun 24, 2020

vinamra28 Jun 24, 2020

vdemeester Jun 24, 2020

ghost Jun 24, 2020

ghost commented Jun 24, 2020

vinamra28 commented Jun 24, 2020

vdemeester left a comment

tekton-robot commented Jun 24, 2020

vinamra28 commented Jun 24, 2020

Fix git-init in case master is not the default branch #2835

Fix git-init in case master is not the default branch #2835

Conversation

vinamra28 commented Jun 18, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Jun 18, 2020

tekton-robot commented Jun 18, 2020

ghost commented Jun 18, 2020

vinamra28 commented Jun 19, 2020

vdemeester Jun 19, 2020

Choose a reason for hiding this comment

vinamra28 Jun 19, 2020

Choose a reason for hiding this comment

vdemeester Jun 19, 2020

Choose a reason for hiding this comment

vinamra28 commented Jun 20, 2020

vdemeester left a comment

Choose a reason for hiding this comment

vinamra28 commented Jun 23, 2020

tekton-robot commented Jun 23, 2020

tekton-robot commented Jun 23, 2020

tekton-robot commented Jun 23, 2020

vinamra28 commented Jun 23, 2020

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Jun 24, 2020

Choose a reason for hiding this comment

vinamra28 Jun 24, 2020

Choose a reason for hiding this comment

vdemeester Jun 24, 2020

Choose a reason for hiding this comment

ghost Jun 24, 2020

Choose a reason for hiding this comment

ghost commented Jun 24, 2020

vinamra28 commented Jun 24, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jun 24, 2020

vinamra28 commented Jun 24, 2020

Fix git-init in case `master` is not the default branch #2835

Fix git-init in case `master` is not the default branch #2835

vinamra28 commented Jun 18, 2020 •

edited

Loading