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

Issue 1182: Add sslVerify flag to pipelineresource type git #1752

Merged
merged 1 commit into from
Dec 19, 2019

Conversation

dibbles
Copy link
Member

@dibbles dibbles commented Dec 16, 2019

Changes

Enables the disabling in git config of http.sslVerify such that the git fetch can succeed against local gitlab installs with self signed certificates by specifying sslVerify as "false" in the pipelineresource.

Fixes #1182

Submitter Checklist

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

See the contribution guide for more details.

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

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 sslVerify parameter is now available on PipelineResource's of type git. This property defines whether http.sslVerify should be set to true or false in the global git config. Setting the property to false will disable certificate validation during the running of the git fetch against the git server and will be of use to people hosting git servers with self signed certificates.
The parameter sslVerify defaults to true if omitted.

Example usage:

apiVersion: tekton.dev/v1alpha1
kind: PipelineResource
metadata:
  name: wizzbang-git
  namespace: default
spec:
  type: git
  params:
    - name: url
      value: https://github.com/wizzbangcorp/wizzbang.git
    - name: revision
      value: master
    - name: sslVerify
      value: "false"

@tekton-robot tekton-robot requested review from a user and vdemeester December 16, 2019 14:37
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Dec 16, 2019
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 16, 2019
@dibbles
Copy link
Member Author

dibbles commented Dec 16, 2019

I noted the existing issue after I had written the code ...... I also have noted there are a number of other issues about git config parameters but thought as I has the code for this one option, I might as well open the pull request to garner interest on taking the change until other solutions might become available.

@dibbles dibbles changed the title Issue 1182: Add sslVerify flag to pipelineresource type git WIP: Issue 1182: Add sslVerify flag to pipelineresource type git Dec 16, 2019
@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2019
cmd/git-init/main.go Outdated Show resolved Hide resolved
@dibbles dibbles force-pushed the tlsVerify branch 2 times, most recently from d1e1122 to 41cf522 Compare December 16, 2019 16:35
@dibbles
Copy link
Member Author

dibbles commented Dec 16, 2019

For reference:- #1182 was found after coding - adding link/comment here.

@dibbles dibbles changed the title WIP: Issue 1182: Add sslVerify flag to pipelineresource type git Issue 1182: Add sslVerify flag to pipelineresource type git Dec 16, 2019
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2019
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

A few minor comments but overall looks great :D A couple requests:

docs/resources.md Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
pkg/git/git.go Show resolved Hide resolved
pkg/git/git.go Show resolved Hide resolved
pkg/reconciler/taskrun/taskrun_test.go Outdated Show resolved Hide resolved
@dibbles
Copy link
Member Author

dibbles commented Dec 17, 2019

Will look over dibyo's review tomorrow

Enables the disabling in git config of http.sslVerify such that
the git fetch can succeed against local gitlab installs with self
signed certificates.
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Thanks @dibbles !!
The go.mod change still seems to be in...is that needed?

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2019
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm
Yeah the go.mod changes are… inherent… for some reason they should have been done in the go.mod PR.. no biggies 👼

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, 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:

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 merged commit 60c8304 into tektoncd:master Dec 19, 2019
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: Add a git parameter in TaskRun object to disable TLS verification from git source
5 participants