Skip to content

Conversation

@piyush-garg
Copy link
Contributor

@piyush-garg piyush-garg commented Oct 13, 2021

This will add a deployment in OpenShift addon which will serve
all the binaries of tkn CLI and installed through installerset
then another installerset will run after the previous is completely run
to install ConsoleCLIDownload which will have the links to the binaries
hosted by the deployment and transformer will be doing that on fly

Add routes permission in role and CSV

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.

Release Notes

Make ConsoleCLiDownload supply binaries from local

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 13, 2021
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 13, 2021
@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/controller.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/extension.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/tektonaddon.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/transformer.go Do not exist 33.3%

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Oct 13, 2021
Comment on lines 93 to 97
{"Linux", "tkn/tkn-linux-amd64-0.19.1.tar.gz"},
{"IBM Power", "tkn/tkn-linux-ppc64le-0.19.1.tar.gz"},
{"IBM Z", "tkn/tkn-linux-s390x-0.19.1.tar.gz"},
{"Mac", "tkn/tkn-macos-amd64-0.19.1.tar.gz"},
{"Windows", "tkn/tkn-windows-amd64-0.19.1.zip"},
Copy link
Contributor

Choose a reason for hiding this comment

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

how about getting tkn version dynamically instead of hardcoding it here

Copy link
Member

Choose a reason for hiding this comment

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

this is the version which have signed binaries, I am not sure how to get those dynamically

Copy link
Member

Choose a reason for hiding this comment

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

but we can use a constant and then use it in there, so whenever we have to change the binary version we do not have to change everywhere

Suggested change
{"Linux", "tkn/tkn-linux-amd64-0.19.1.tar.gz"},
{"IBM Power", "tkn/tkn-linux-ppc64le-0.19.1.tar.gz"},
{"IBM Z", "tkn/tkn-linux-s390x-0.19.1.tar.gz"},
{"Mac", "tkn/tkn-macos-amd64-0.19.1.tar.gz"},
{"Windows", "tkn/tkn-windows-amd64-0.19.1.zip"},
const Version = "0.19.1"
platforms := []struct {
label string
key string
}{
{"Linux for x86_64", "tkn/tkn-linux-amd64-"+Version+".tar.gz"},
{"Linux for IBM Power", "tkn/tkn-linux-ppc64le-"+Version+".tar.gz"},
{"Linux for IBM Z", "tkn/tkn-linux-s390x-"+Version+".tar.gz"},
{"Mac for x86_64", "tkn/tkn-macos-amd64-"+Version+".tar.gz"},
{"Windows for x86_64", "tkn/tkn-windows-amd64-"+Version+".zip"},
}

Copy link
Member

Choose a reason for hiding this comment

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

we can read that version from deployment env, so everytime we can update in CSV similar to images

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be handled using a configMap. These links dependent on an external system (the binaries copied in the dist-git server build). if we can capture this data in ta configMap, with platform keys and the url fragment as value, then we can replace the values in config map outside of an operator buidl. ie, we will not have to make a version bump pr to this repo.

in otherwords, this is a detail which will change based on release branches, so it will be the part of release-scripts/pipelines to edit the placehoder values we mention in this configmap.

🧑‍💻 i think we can merge this as it is for now as we are on a short schedule.

@piyush-garg could you create an issue, so that we can handle this in a later pr.

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/controller.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/extension.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/tektonaddon.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/transformer.go Do not exist 33.3%

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/controller.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/extension.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/tektonaddon.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/transformer.go Do not exist 33.3%

@nikhil-thomas
Copy link
Member

LGTM

This will add a deployment in OpenShift addon which will serve
all the binaries of tkn CLI and installed through installerset
then another installerset will run after the previous is completely run
to install ConsoleCLIDownload which  will have the links to the binaries
hosted by the deployment and transformer will be doing that on fly

Add routes permission in role and CSV

Add the env in csv for image replacement and added the transformer
for imagereplacement in tektonaddon
@piyush-garg
Copy link
Contributor Author

Added the part for image replacement also. added the env and transformer in tektonaddon.

@tekton-robot
Copy link
Contributor

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

File Old Coverage New Coverage Delta
pkg/reconciler/openshift/tektonaddon/controller.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/extension.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/tektonaddon.go Do not exist 0.0%
pkg/reconciler/openshift/tektonaddon/transformer.go Do not exist 33.3%

@sm43
Copy link
Member

sm43 commented Oct 14, 2021

/lgtm
/meow

@tekton-robot
Copy link
Contributor

@sm43: cat image

Details

In response to this:

/lgtm
/meow

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 tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2021
@tekton-robot tekton-robot merged commit f600b95 into tektoncd:main Oct 14, 2021
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

8 participants