Rewrite git-clone as shell script #188

chmouel · 2020-02-19T17:23:33Z

We are rewritting git-clone as a 'pure' bourne shell script with no dependence
on the git-init image.

I have rewritten almost word by word the go code from here https://git.io/JvBCr

Error handling are handled by shell's set -e so if any command fails it will
just exit 1,

There is some bugs in go code submodules which is done always so i have handled
this differently to properly be conditional to the parameter.

Changes

Submitter Checklist

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

Includes docs (if user facing)
Commit messages follow commit message best practices
Yaml file complies with yamllint rules.

See the contribution guide
for more details.

ghost · 2020-02-19T17:57:01Z

git/git-clone.yaml

+          if [[ ${DEPTH} > 0 ]];then
+              fetchArgs="${fetchArgs} --depth=${DEPTH}"
+          fi
+          if git ${fetchArgs};then


Missing [[ ]]?

Actually maybe I misunderstand what this is doing. What does this do?

It will check if the command exit properly and if it doesn't it will fallback, I have simply converted the GO code, and I assume that fallback was written to cover a user case... i.e:

https://github.com/tektoncd/pipeline/blob/4e9380fe0d0e7ce862ff23781f614ad8e6de9faf/pkg/git/git.go#L91-L92

ghost

I tried this out locally in a pipeline and it performed the same as the existing git-init binary (as far as I can tell...)

git/git-clone.yaml

tekton-robot · 2020-02-19T18:01:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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]

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

ghost · 2020-02-19T18:11:29Z

git/git-clone.yaml

+              fetchArgs="${fetchArgs} --depth=${DEPTH}"
+          fi
+          if git ${fetchArgs};then
+              git pull origin master


I think it's possible that there is no "master" branch on the remote.

What is the purpose of this pull? We fetch in the previous line and then we checkout the specific revision on the next line.

yeah i wondered it too (to be honest I merely converted the GO code without too much tweaking), i think we can just git reset --hard anyway...

chmouel · 2020-02-24T08:01:14Z

Adding @sthaha for advice since he is the git guru 😀

git/git-clone.yaml

sthaha

Looks good to me; may want to consider addressing some of the comments.

sthaha · 2020-02-25T00:05:14Z

git/git-clone.yaml

+
+      ensureHomeEnv
+      fetch
+      echo -n "$(git rev-parse HEAD | tr -d '\n')" > "$(results.commit.path)"


echo isn't needed - right? i.e. git rev-parse HEAD | tr -d '\n' > "$( ...) should work - right?

sthaha · 2020-02-25T00:08:42Z

git/git-clone.yaml

+      # https://git.io/JvBCr
+      fetch() {
+          [[ ${REVISION} == "" ]] && REVISION=master
+          if [[ ${CHECKOUT_DIR} != "" ]];then


nit: this can reduced to

test -z "$CHECKOUT_DIR" || cd "$CHECKOUT_DIR" git init .

sthaha · 2020-02-25T00:15:48Z

git/git-clone.yaml

+          else
+              git init .
+          fi
+          trimmedURL=$(echo ${URL}|sed -e 's/^[ ]*//' -e 's/[ ]*$//')


also just curious ... wouldn't trimmedURL=$(echo $URL) work :D ?

that doesn't trim the spaces:

@chmouel are you using sh? or zsh?

@chmouel you don't have to change the implementation, I was only curious ...

chmouel · 2020-03-03T13:03:56Z

/hold

will do this rewrite when i get a bit of time..

sthaha · 2020-03-03T23:47:35Z

/hold

will do this rewrite when i get a bit of time..

IMHO, we can merge this while you work on the improved version.

bobcatfish · 2020-03-13T17:07:49Z

I'm gonna contradict tektoncd/pipeline#1961:

The disadvantage of this is that git-type PipelineResources become dependent on (presumably) Bash scripts, which can grow to unmaintainable sizes. Unit testing is hard in Bash, but git-init has no unit tests today anyway.

I'm not sure that a script is the most maintainable way forward here? As soon as we get beyond a few lines in a script, whether we're talking about python or git, and when we're talking about a piece of infra that is so core to so many Pipelines (i.e. pulling from git), I feel like we want to start having testing, linting, all that good stuff. Moving this logic into a script feels like a couple steps backward for me?

It definitely has the advantage of being able to see exactly what the script is doing, but I'd rather that we start adding tests and debug info to the git-init logic we already have - even move it into its own repo (or into this one).

/hold

chmouel · 2020-03-19T10:33:37Z

@bobcatfish that's some fair points here is some of the advantages I see using a only shell git clone task :

It's easier for user to debug what's going on instead of having to dig deep into the code, figuring out which git command has been run.
It's easier for the user to modify, if they need some esoteric way of doing git-clonning they could just modify this task.
It's easier for distributors to maintain and having one image less.
There is no dependency between the go code and the task and having to make sure task version X depends on git-init version Y.
The current git-init image is mostly launching shell commands and not doing much more what a shell script is doing. It's arguably as difficult to read the git-init code than the shell script (or at least it was for me when translating the go code in shell).

And to add this, I am adding tests for this and we can have a job doing bash unit tests.

vdemeester · 2020-03-19T13:08:04Z

As soon as we get beyond a few lines in a script, whether we're talking about python or git, and when we're talking about a piece of infra that is so core to so many Pipelines (i.e. pulling from git), I feel like we want to start having testing, linting, all that good stuff. Moving this logic into a script feels like a couple steps backward for me?

I am not entirely sure how "so core" it is to Pipelines. It can be considered as core if it is in tektoncd/pipeline as the git PipelineResource git. But cloning a repository in a task, is at the same level as any other task that Pipelines are supposed to support, it shouldn't be consider as core as it is just a way to populate a workspace to be consumed by later Tasks — at least this task shouldn't (but whatever is used by the git PipelineResource now or in the future, should).

I do agree (and adding 👍) with @chmouel on his points, the git-init image is more opaque than a git script, and both do the same in the end (aka executing git commands).

We are rewritting git-clone as a 'pure' bourne shell script with no dependence on the git-init image. I have rewritten almost word by word the go code from here https://git.io/JvBCr Error handling are handled by shell's set -e so if any command fails it will just exit 1, There is some bugs in go code submodules which is done always so i have handled this differently to properly be conditional to the parameter. Signed-off-by: Chmouel Boudjnah <[email protected]>

chmouel · 2020-03-19T14:04:02Z

I have added some tests and resynced with master...

tekton-robot · 2020-03-23T08:43:12Z

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

Test name	Commit	Details	Rerun command
pull-tekton-catalog-integration-tests	`87041b8`	link	`/test pull-tekton-catalog-integration-tests`

Full PR test history. Your PR dashboard.

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.

…o-release-v0.11 New merge from upstream v1beta to release v0.11

ghost · 2020-04-17T12:56:59Z

@bobcatfish @chmouel @vdemeester can we find a venue to discuss this further and get closure for this PR? Perhaps the Pipelines WG? I think there are sound arguments on both sides of this debate and we should probably resolve them so that we can set the policy for other catalog entries going forward.

chmouel · 2020-04-20T12:59:10Z

Sounds good to me, let's have this discussion on next week agenda.

chmouel · 2020-06-10T11:58:09Z

/close

let's try in another way maybe complicated shell scripts is a bit too cryptic, will send a new PR trying it in Python+Tests.

tekton-robot · 2020-06-10T11:58:11Z

@chmouel: Closed this PR.

In response to this:

/close

let's try in another way maybe complicated shell scripts is a bit too cryptic, will send a new PR trying it in Python+Tests.

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 requested review from kimsterv and vdemeester February 19, 2020 17:23

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 19, 2020

ghost reviewed Feb 19, 2020

View reviewed changes

ghost approved these changes Feb 19, 2020

View reviewed changes

git/git-clone.yaml Show resolved Hide resolved

git/git-clone.yaml Outdated Show resolved Hide resolved

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

ghost reviewed Feb 19, 2020

View reviewed changes

chmouel force-pushed the git-as-script branch from 3729608 to f87e20e Compare February 24, 2020 08:10

sthaha reviewed Feb 24, 2020

View reviewed changes

git/git-clone.yaml Outdated Show resolved Hide resolved

sthaha reviewed Feb 25, 2020

View reviewed changes

git/git-clone.yaml Show resolved Hide resolved

sthaha reviewed Feb 25, 2020

View reviewed changes

git/git-clone.yaml Outdated Show resolved Hide resolved

sthaha reviewed Feb 25, 2020

View reviewed changes

vdemeester mentioned this pull request Feb 26, 2020

Q: Migrate git-init to script mode? tektoncd/pipeline#1961

Closed

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2020

ghost mentioned this pull request Mar 12, 2020

adding proxy parameters to git pipeline resource tektoncd/pipeline#2215

Merged

3 tasks

jace-ys mentioned this pull request Mar 17, 2020

git-clone tutorial failing #224

Closed

chmouel force-pushed the git-as-script branch from f87e20e to 3c45fb4 Compare March 19, 2020 14:00

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 Mar 19, 2020

chmouel force-pushed the git-as-script branch from 3c45fb4 to ffc2388 Compare March 19, 2020 14:01

Fix showing container logs in e2e tests

21a9f34

Allow success detection when you have multiple tasks in the test

87041b8

nikhil-thomas pushed a commit to nikhil-thomas/catalog that referenced this pull request Apr 13, 2020

Merge pull request tektoncd#188 from chmouel/merge2-upstream-v1beta-t…

2775542

…o-release-v0.11 New merge from upstream v1beta to release v0.11

tekton-robot closed this Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite git-clone as shell script #188

Rewrite git-clone as shell script #188

chmouel commented Feb 19, 2020

ghost Feb 19, 2020

ghost Feb 19, 2020

chmouel Feb 24, 2020

ghost left a comment

tekton-robot commented Feb 19, 2020

ghost Feb 19, 2020

chmouel Feb 24, 2020 •

edited

Loading

chmouel commented Feb 24, 2020

sthaha left a comment

sthaha Feb 25, 2020

sthaha Feb 25, 2020

sthaha Feb 25, 2020

chmouel Mar 19, 2020

sthaha Mar 20, 2020 •

edited

Loading

sthaha Mar 20, 2020

sthaha Mar 20, 2020

chmouel commented Mar 3, 2020

sthaha commented Mar 3, 2020

bobcatfish commented Mar 13, 2020

chmouel commented Mar 19, 2020 •

edited

Loading

vdemeester commented Mar 19, 2020

chmouel commented Mar 19, 2020

tekton-robot commented Mar 23, 2020

ghost commented Apr 17, 2020

chmouel commented Apr 20, 2020

chmouel commented Jun 10, 2020

tekton-robot commented Jun 10, 2020

Rewrite git-clone as shell script #188

Rewrite git-clone as shell script #188

Conversation

chmouel commented Feb 19, 2020

Changes

Submitter Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 19, 2020

Choose a reason for hiding this comment

chmouel Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

chmouel commented Feb 24, 2020

sthaha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chmouel commented Mar 3, 2020

sthaha commented Mar 3, 2020

bobcatfish commented Mar 13, 2020

chmouel commented Mar 19, 2020 • edited Loading

vdemeester commented Mar 19, 2020

chmouel commented Mar 19, 2020

tekton-robot commented Mar 23, 2020

ghost commented Apr 17, 2020

chmouel commented Apr 20, 2020

chmouel commented Jun 10, 2020

tekton-robot commented Jun 10, 2020

chmouel Feb 24, 2020 •

edited

Loading

sthaha Mar 20, 2020 •

edited

Loading

chmouel commented Mar 19, 2020 •

edited

Loading