Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove implicit "set -x" from git-clone, deleteExisting by default #433

Merged
merged 1 commit into from Aug 14, 2020
Merged

Remove implicit "set -x" from git-clone, deleteExisting by default #433

merged 1 commit into from Aug 14, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jul 22, 2020

Changes

This commit adds git-clone v0.2. One default behaviour has changed
and one new param has been introduced in this version:

  1. Tasks that don't start their script with #! automatically receive
    a preamble that includes set -x. This results in verbose output
    because every command is printed while the Task is running.

This commit adds a new param, "verbose", that - when set to "false" -
prevents the commands run by the task from being printed to logs.
The default value is "true": the commands run by the task will be
printed in the log.

  1. The git-clone Task offers a param, deleteExisting, that cleans
    any existing directory before performing a clone into it. This
    has previously been set to "false" by default since it's potentially
    destructive. But it sounds like there's quite often a request for
    how to enable this behaviour, so it seems like the more pragmatic
    default would be "true".

This commit sets deleteExisting to be defaulted to "true" so that
any existing git checkout is removed prior to a new clone being
performed.

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.
  • Complies with [Catalog Orgainization TEP][TEP], see [example]. Note [An issue has been filed to automate this validation][validation]
    • File path follows <kind>/<name>/<version>/name.yaml
    • Has README.md at <kind>/<name>/<version>/README.md
    • Has mandatory metadata.labels - app.kubernetes.io/version the same as the <version> of the resource
    • Has mandatory metadata.annotations tekton.dev/pipelines.minVersion
    • mandatory spec.description follows convention

@ghost ghost requested review from chmouel and sthaha July 22, 2020 15:26
@tekton-robot tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 22, 2020
@ghost
Copy link
Author

ghost commented Jul 22, 2020

/hold

I will add some tests around deleteExisting "true" and "false"

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2020
@bobcatfish
Copy link
Contributor

Tasks that don't start their script with #! automatically receive a preamble that includes set -x. This results in verbose output because every command is printed while the Task is running.

@sbwsg could you explain more about why we want to remove this? i find being able to have this kind of verbose output really helps with debugging, and one of the difficulties I have with a lot of our e2e bash scripts is that they do not do this and it's very hard to know what they actually executing

@ghost
Copy link
Author

ghost commented Jul 22, 2020

@sbwsg could you explain more about why we want to remove this?

This stems from a conversation on Slack with @chmouel who suggested reducing the amount of log noise to make things a bit less confusing. I guess when the script is working users don't really want or need to see the noise. But when the script isn't working it can be helpful to get lengthier log output.

I could add a param like verbose which defaults to "false". Making it "true" could result in set -x being added. WDYT @bobcatfish @chmouel ?

image: gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/git-init:v0.14.2
script: |
#!/bin/sh
set -e
Copy link
Member

Choose a reason for hiding this comment

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

as a rule of thumb, I think we should have set -eu set by default if not more stricter set -eu -o pipefail for all bash scripts.

cd "$CHECKOUT_DIR"
RESULT_SHA="$(git rev-parse HEAD | tr -d '\n')"
EXIT_CODE="$?"
if [ "$EXIT_CODE" != 0 ]
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice if we followed the same convention as above

if [[ "$EXIT_CODE" != 0 ]] ; then 
   exit $EXIT_CODE
fi

-submodules="$(params.submodules)" \
-depth "$(params.depth)"
cd "$CHECKOUT_DIR"
RESULT_SHA="$(git rev-parse HEAD | tr -d '\n')"
Copy link
Member

@sthaha sthaha Jul 23, 2020

Choose a reason for hiding this comment

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

isn't | tr -d '\n' unnecessary? 🤔
could you try echo "$(git rev-parse HEAD) |" ?

Copy link
Member

Choose a reason for hiding this comment

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

@sthaha I think you meant :

Suggested change
RESULT_SHA="$(git rev-parse HEAD | tr -d '\n')"
RESULT_SHA="$(echo -n git rev-parse HEAD)"

which should do the same as @sbwsg in a slightly more elegant way..

Copy link
Member

@sthaha sthaha Jul 23, 2020

Choose a reason for hiding this comment

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

@chmouel no, I meant RESULT_SHA=$(git rev-parse HEAD)

The echo was to just show that it doesn't insert a \n or is this dependent on the git version ??

Copy link
Member

Choose a reason for hiding this comment

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

yeah you right either way it's not needed to cut the \n....

@chmouel
Copy link
Member

chmouel commented Jul 23, 2020

I could add a param like verbose which defaults to "false". Making it "true" could result in set -x being added. WDYT @bobcatfish @chmouel ?

I just think that having verbosity by default was confusing,

this maybe could be a pipeline level sort of option when we have script with no shbang, let the user in some way specify she or he may wants to run it in a verbose way?

+1 on scott's suggestions in the meantime.

@bobcatfish
Copy link
Contributor

I just think that having verbosity by default was confusing,

It's a tough balance to strike: when you don't need the output, it's confusing, however when you do need it (and you often don't know that until it's too late), it's really handy to have it.

The e2e test scripts we have been using across Tekton so far have taken the stance that less is better and have not included -x. I've found this makes debugging them, maintaining them, and updating them extremely hard.

If we want to add a flag to disable this, I suggest we default the functionality to being more verbose by default.

Note that script mode defaults to -x as well https://github.com/tektoncd/pipeline/blob/master/docs/tasks.md#running-scripts-within-steps

@ghost
Copy link
Author

ghost commented Jul 28, 2020

OK, I've updated with - I think - all suggested modifications. I've also introduced a verbose param to make the set -x behaviour configurable.

/hold cancel

/test pull-tekton-catalog-integration-tests

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2020
Comment on lines 62 to 70
podAntiAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
- labelSelector:
matchExpressions:
- key: "tekton.dev/pipelineRun"
operator: In
values:
- git-clone-checking-out-a-branch
topologyKey: kubernetes.io/hostname
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I understand this part, is it to make sure the pipelineRuns doesn't end up on the same minion ?

Copy link
Member

Choose a reason for hiding this comment

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

I understand the same as @chmouel, is it to make sure we don't run on the same minion of that task because they might "hit on each others with PVC or sthg" ?

Copy link
Author

Choose a reason for hiding this comment

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

this was copied verbatim from the 0.1 version. Looks like it might be a way to manage the pvcs pre-affinityassistant like you say. I'll go back and take a better look.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed the affinity, let's see how this runs in integration.

@ghost
Copy link
Author

ghost commented Jul 30, 2020

OK looks like that worked out ok without the affinity rules. This should be ready for re-review.

Copy link
Member

@chmouel chmouel left a comment

Choose a reason for hiding this comment

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

/lgtm

Looking Good! 🤙🏽

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2020
@vdemeester
Copy link
Member

/lgtm

@ghost
Copy link
Author

ghost commented Aug 14, 2020

Hey this is just waiting on an approve. Do any reviewers have more comments before it proceeds?

Copy link
Contributor

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

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

sorry a bit more feedback but nothing blocking!!

/approve

* **deleteExisting**: clean out the contents of the repo's destination directory if it already exists before cloning the repo there (_default_: true)
* **httpProxy**: git HTTP proxy server for non-SSL requests
* **httpsProxy**: git HTTPS proxy server for SSL requests
* **noProxy**: git no proxy - opt out of proxying HTTP/HTTPS requests
Copy link
Contributor

Choose a reason for hiding this comment

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

are these optional? i think it would make sense to mention a default for them if so? i also think it might be useful to list the required params before the optional ones

Copy link
Contributor

Choose a reason for hiding this comment

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

i think "verbose" is missing from this list

(but also i think maintaining this list AND the docs in the Task itself is strange, but that's a larger question!)

Copy link
Author

Choose a reason for hiding this comment

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

Added "verbose". Added the default values for the proxy params. Sort order has required params at top.

- name: verbose
description: log the commands used during execution
type: string
default: "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can add bool types some day

* **depth**: performs a shallow clone where only the most recent commit(s) will be fetched (_default_: 1)
* **sslVerify**: defines if http.sslVerify should be set to true or false in the global git config (_default_: true)
* **subdirectory**: subdirectory inside the "output" workspace to clone the git repo into (_default:_ "")
* **deleteExisting**: clean out the contents of the repo's destination directory if it already exists before cloning the repo there (_default_: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it might make sense to highlight this one more - i wouldnt expect this behavior personally and now this is the default

Copy link
Author

Choose a reason for hiding this comment

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

Added an additional bit of prose to the README calling out this default behaviour.

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2020
@ghost
Copy link
Author

ghost commented Aug 14, 2020

ok if integration fails I'll make the suggested changes in this PR otherwise I'll open a new one.

@bobcatfish
Copy link
Contributor

{"level":"fatal","ts":1597417297.477715,"logger":"fallback-logger","caller":"imagedigestexporter/main.go:78","msg":"Unexpected error writing message .//image-digested to open .//image-digested: permission denied","commit":"6d12d1e","stacktrace":"main.main\n\tgithub.com/tektoncd/pipeline/cmd/imagedigestexporter/main.go:78\nruntime.main\n\truntime/proc.go:203"}
2020/08/14 15:01:37 Skipping step because a previous step failed
2020/08/14 15:01:40 Exiting...

Did we just upgrade the version of tekton we're testing with and hit a bug maybe?

@bobcatfish
Copy link
Contributor

/test pull-tekton-catalog-integration-tests

This commit adds git-clone v0.2. One default behaviour has changed
and one new param has been introduced in this version:

1) Tasks that don't start their script with #! automatically receive
a preamble that includes `set -x`. This results in verbose output
because every command is printed while the Task is running.

This commit adds a new param, "verbose", that - when set to "false"
- prevents the commands run by the task from being printed to logs.
The default value is "true": the commands run by the task will be
printed in the log.

2) The git-clone Task offers a param, deleteExisting, that cleans
any existing directory before performing a clone into it. This
has previously been set to "false" by default since it's potentially
destructive. But it sounds like there's quite often a request for
how to enable this behaviour, so it seems like the more pragmatic
default would be "true".

This commit sets deleteExisting to be defaulted to "true" so that
any existing git checkout is removed prior to a new clone being
performed.
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2020
@bobcatfish
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2020
@tekton-robot tekton-robot merged commit 9a4c087 into tektoncd:master Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants