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

add tekton chart for installation #100

Merged
merged 5 commits into from Aug 5, 2021

Conversation

wujiahao15
Copy link
Contributor

@wujiahao15 wujiahao15 commented Aug 4, 2021

resolves #62

Description:

  • Add tekton pipeline chart into ks-devops for installation of tekton.
  • We can use make install-chart in command line to install Jenkins, s2i and tekton-pipeline.
  • After successfully installing the chart, the tekton components are deployed in the namespace called tekton-pipelines.

@codecov-commenter
Copy link

Codecov Report

Merging #100 (6a21e56) into master (bdd31ea) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #100   +/-   ##
======================================
  Coverage    5.34%   5.34%           
======================================
  Files          74      74           
  Lines       21072   21072           
======================================
  Hits         1126    1126           
  Misses      19873   19873           
  Partials       73      73           
Flag Coverage Δ
unittests 5.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7714e9c...6a21e56. Read the comment docs.

@ks-ci-bot ks-ci-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 4, 2021
Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. But IMO, we don't have a good reason to maintain the Tekton helm chart. So, it's not a good idea to commit the source files of it into this repository. I have two suggestions here:

  • commit and commit a specific release helm chart of Tekton
    • it should be a file likes xxx.tgz
  • create a PR against the branch tekton-support

@wujiahao15
Copy link
Contributor Author

wujiahao15 commented Aug 4, 2021

Thanks for your contribution. But IMO, we don't have a good reason to maintain the Tekton helm chart. So, it's not a good idea to commit the source files of it into this repository. I have two suggestions here:

  • commit and commit a specific release helm chart of Tekton

    • it should be a file likes xxx.tgz
  • create a PR against the branch tekton-support

Okay, Master.

@JohnNiang
Copy link
Member

Thanks for your contribution. But I have a question for you: what should we do if we want to install ks-devops without tekton?

@wujiahao15
Copy link
Contributor Author

wujiahao15 commented Aug 4, 2021

Thanks for your contribution. But I have a question for you: what should we do if we want to install ks-devops without tekton?

How about adding condition field in the parent Chart.yaml to controll dependecies and adding charts.xxx.enabled in values.yaml?
e.g.

# Chart.yaml
...
dependencies:
  - name: jenkins
    version: 0.19.0
    condition: charts.jenkins.enabled
  - name: s2i
    version: 1.16.0
    condition: charts.s2i.enabled
  - name: tekton-pipeline
    version: 0.25.0
    condition: charts.tekton.enabled
...

# values.yaml
...
charts:
  jenkins:
    enabled: true
  s2i:
    enabled: true
  tekton:
    enabled: false
...

In this case, tekton is not installed by default. If we want to install tekton altogether, we can overwrite the value charts.tekton.enabled as true before installation.

i.e we could provide an alternative command in README.md, just like changing the container registry from gcr.io to docker.io.

e.g. adding the below message to the section of Install it as a Helm Chart in README.md

- Because tekton is not installed by default, if you want to install it, you can exec `helm install ks-devops chart/ks-devops --set charts.tekton.enabled=true`

@JohnNiang
Copy link
Member

@flamywhale Thanks for your patient explanation. That makes sense to me.

@wujiahao15
Copy link
Contributor Author

Thanks for your contribution. But IMO, we don't have a good reason to maintain the Tekton helm chart. So, it's not a good idea to commit the source files of it into this repository. I have two suggestions here:

  • commit and commit a specific release helm chart of Tekton

    • it should be a file likes xxx.tgz
  • create a PR against the branch tekton-support

I am little confused about the second suggestion in which I am supposed to create a PR against the branch tekton-support.
Do I need to close this PR and create a new one for the branch tekton-support?

@wujiahao15 wujiahao15 changed the base branch from master to tekton-support August 5, 2021 02:47
@JohnNiang
Copy link
Member

hi @flamywhale , you can edit anything of this PR directly.

Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

Everything is perfect. But it would be great if you can add a description about how (or where) do you get the chart zip file. For example, add a readme file under the directory: charts/ks-devops/charts/

@wujiahao15
Copy link
Contributor Author

hi @flamywhale , you can edit anything of this PR directly.

Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

❗ No coverage uploaded for pull request base (tekton-support@7714e9c). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 9b4574a differs from pull request most recent head 993707e. Consider uploading reports for the commit 993707e to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##             tekton-support    #100   +/-   ##
================================================
  Coverage                  ?   5.34%           
================================================
  Files                     ?      74           
  Lines                     ?   21072           
  Branches                  ?       0           
================================================
  Hits                      ?    1126           
  Misses                    ?   19873           
  Partials                  ?      73           
Flag Coverage Δ
unittests 5.34% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7714e9c...993707e. Read the comment docs.

@ks-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flamywhale
To complete the pull request process, please assign linuxsuren after the PR has been reviewed.
You can assign the PR to them by writing /assign @linuxsuren in a comment when ready.

The full list of commands accepted by this bot can be found 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

@JohnNiang
Copy link
Member

/label tide/merge-method-squash

Copy link
Member

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

/lgtm

@LinuxSuRen
Copy link
Member

/label tide/merge-method-squash

It looks like the Prow has same problems.

@JohnNiang
Copy link
Member

/label tide/merge-method-squash

@JohnNiang
Copy link
Member

/lgtm

@ks-ci-bot
Copy link
Collaborator

@JohnNiang: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@LinuxSuRen LinuxSuRen merged commit b082f15 into kubesphere:tekton-support Aug 5, 2021
LinuxSuRen pushed a commit that referenced this pull request Aug 16, 2021
LinuxSuRen pushed a commit that referenced this pull request Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ospp-2021 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Tekton charts into ks-devops
5 participants