Store tektoncd/pipeline release version to a variable during build #1650

waveywaves · 2019-11-29T11:52:12Z

Changes

Fixes Add version label to pipelines controller deployment #1543
Sets the PipelineVersion variable value through golang-build task used during release.
Solution to Feature Request: tkn pipeline version cli#463 would augment this, by using the cli to show forth the value for PipelineVersion which would be the release version for tektoncd/pipeline

Submitter Checklist

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

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

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

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

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

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

tekton-robot · 2019-11-29T11:52:27Z

Hi @waveywaves. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

waveywaves · 2019-11-29T11:52:30Z

cc @chmouel @vdemeester

vdemeester

The main problem with that approach is that, the final binaries are built using ko and not the go build task from the release pipeline. This means, on released version (or even nightly), the version will always be "dev" 😓

waveywaves · 2019-11-29T13:03:48Z

@vdemeester Can I add ldflags to ko build then ? :p

vdemeester · 2019-11-29T13:10:51Z

@vdemeester Can I add ldflags to ko build then ? :p

I don't know if it's possible, but if it is, yeah 😅

waveywaves · 2019-11-29T13:53:01Z

@vdemeester It is possible. Here's an example of the same. ko-build/ko#54 (comment)

But it seems that we are not building a binary directly due to which it seems hard to do or if it is being created.. is https://github.com/tektoncd/pipeline/blob/master/tekton/resources.yaml#L109 using it ?

waveywaves · 2019-11-29T14:00:02Z

Also another approach would be to use the github api, should I try that instead ? Based on that I am able to query https://api.github.com/repos/tektoncd/cli/releases/latest but querying https://api.github.com/repos/tektoncd/pipeline/releases/latest is giving me a not found message, is api access for latest release not enabled there ?

vdemeester · 2019-11-29T14:33:28Z

@vdemeester It is possible. Here's an example of the same. google/ko#54 (comment)

Oh nice 🎉 We should have that in tektoncd/pipeline then 👼

But it seems that we are not building a binary directly due to which it seems hard to do or if it is being created.. is https://github.com/tektoncd/pipeline/blob/master/tekton/resources.yaml#L109 using it ?

I didn't understand this question 😛. The resource is just to have the image tag, nothing else.

Also another approach would be to use the github api, should I try that instead ? Based on that I am able to query https://api.github.com/repos/tektoncd/cli/releases/latest but querying https://api.github.com/repos/tektoncd/pipeline/releases/latest is giving me a not found message, is api access for latest release not enabled there ?

I wouldn't go there, we shouldn't need to query and depend on GitHub API to be able to add the version in our binary, the information is already in the git repository, no need to depend on GitHub.

waveywaves · 2019-11-30T15:40:07Z

Oh nice 🎉 We should have that in tektoncd/pipeline then 👼

But do we create an explicit binary which we can augment with ldflags ? If there is then it would be easy I wasnt able to find it 😞 (1650.1)

I didn't understand this question 😛. The resource is just to have the image tag, nothing else.

Sorry for that. I just wanted to if there is a binary being create. (which I wasn't able to fine if there was an intermediate one being created) (1650.2)

I wouldn't go there, we shouldn't need to query and depend on GitHub API to be able to add the version in our binary, the information is already in the git repository, no need to depend on GitHub.

That makes sense, something even I wouldn't "prefer" to do as it involves being hacky if you will. But, https://github.com/tektoncd/cli/blob/2ca6adbe0168d3c8da79a8891709b5138cd7f49d/pkg/cmd/version/version.go seemed to do it, hence I asked. (1650.3)

If all else fails and the binary for the controller in (1650.1) doesn't really exist, what is the next step for this solution in terms of solving the problem to tekton version in the controller ?

chmouel · 2019-12-01T10:14:20Z

That makes sense, something even I wouldn't "prefer" to do as it involves being hacky if you will. But, https://github.com/tektoncd/cli/blob/2ca6adbe0168d3c8da79a8891709b5138cd7f49d/pkg/cmd/version/version.go seemed to do it, hence I asked. (1650.3)

That's a common pattern for CLIs to check the binary version, i.e: minikube check-version, not usual for backend/servers....

vdemeester · 2019-12-02T09:12:32Z

But do we create an explicit binary which we can augment with ldflags ? If there is then it would be easy I wasnt able to find it disappointed (1650.1)

Any package in cmd/ becomes a binary that is then baked into an image

afrittoli

Thank you for this.
Versions is definitely something I would like to improve on from where we are today.
I'm not sure this will work with our release process today, but it is possible to pick the tag from the pipeline parameter instead of $(git describe), so this should still be doable.

tekton/release-pipeline.yaml

.ko.yaml

.ko.yaml.release

afrittoli

Thanks for the updates. This looks good now!
Only one comment on the nightly task please.

tekton/publish-nightly.yaml

afrittoli · 2019-12-03T11:55:33Z

/ok-to-test

vdemeester

Looks good :)

tekton/publish-nightly.yaml

tekton-robot · 2019-12-03T13:19:34Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

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

afrittoli

Thanks. This looks good now, only the missing 's' needs fixing.
It would be nice to document how to query the version from the binaries and/or from the running containers. I understand that the CLI plans to provide an nice way to query it, but we should also doc how to do so using only k8s tools.

tekton/publish-nightly.yaml

Fixes tektoncd#1543 Introduces variable for storing the pipeline version

afrittoli

Thank you!
It would be great if you could follow up with docs on this.
/lgtm

As a followup to tektoncd#1650 , the `tekton.dev/release` annotation is set on the pod to reflect the value of version.PipelineVersion Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>

As a followup to #1650 , the `tekton.dev/release` annotation is set on the pod to reflect the value of version.PipelineVersion Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 29, 2019

tekton-robot requested review from abayer and afrittoli November 29, 2019 11:52

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 29, 2019

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 29, 2019

waveywaves force-pushed the update/pipeline-version branch from 24275b3 to 4aff5fb Compare November 29, 2019 11:53

vdemeester reviewed Nov 29, 2019

View reviewed changes

afrittoli reviewed Dec 2, 2019

View reviewed changes

tekton/release-pipeline.yaml Outdated Show resolved Hide resolved

waveywaves force-pushed the update/pipeline-version branch from 4aff5fb to 41edcb3 Compare December 3, 2019 06:41

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2019

afrittoli reviewed Dec 3, 2019

View reviewed changes

tekton/release-pipeline.yaml Show resolved Hide resolved

.ko.yaml Outdated Show resolved Hide resolved

.ko.yaml.release Outdated Show resolved Hide resolved

waveywaves force-pushed the update/pipeline-version branch 4 times, most recently from c816ac7 to 3d5ebac Compare December 3, 2019 11:31

afrittoli reviewed Dec 3, 2019

View reviewed changes

tekton/publish-nightly.yaml Outdated Show resolved Hide resolved

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 3, 2019

waveywaves force-pushed the update/pipeline-version branch 4 times, most recently from 4012daa to b5a13cb Compare December 3, 2019 12:26

vdemeester approved these changes Dec 3, 2019

View reviewed changes

tekton/publish-nightly.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 Dec 3, 2019

afrittoli reviewed Dec 3, 2019

View reviewed changes

tekton/publish-nightly.yaml Outdated Show resolved Hide resolved

tekton/publish-nightly.yaml Outdated Show resolved Hide resolved

Set Pipeline Version during build

ea063f8

Fixes tektoncd#1543 Introduces variable for storing the pipeline version

waveywaves force-pushed the update/pipeline-version branch from b5a13cb to ea063f8 Compare December 3, 2019 15:08

afrittoli reviewed Dec 4, 2019

View reviewed changes

tekton-robot assigned afrittoli Dec 4, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2019

tekton-robot merged commit f85c53f into tektoncd:master Dec 4, 2019

waveywaves mentioned this pull request Dec 16, 2019

Feature to serve the version using /version endpoint knative/pkg#948

Closed

waveywaves mentioned this pull request Dec 17, 2019

Reflect tektoncd/pipeline release version as an annotation on pod #1758

Merged

3 tasks

waveywaves mentioned this pull request Mar 19, 2020

Add waveywaves (Vibhav Bobade (me)) to the org tektoncd/community#87

Merged

bobcatfish mentioned this pull request Jul 21, 2020

Remove the "baseBuildOverrides" flag from .ko.yaml #2972

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store tektoncd/pipeline release version to a variable during build #1650

Store tektoncd/pipeline release version to a variable during build #1650

waveywaves commented Nov 29, 2019

tekton-robot commented Nov 29, 2019

waveywaves commented Nov 29, 2019

vdemeester left a comment

waveywaves commented Nov 29, 2019

vdemeester commented Nov 29, 2019

waveywaves commented Nov 29, 2019

waveywaves commented Nov 29, 2019

vdemeester commented Nov 29, 2019

waveywaves commented Nov 30, 2019 •

edited

Loading

chmouel commented Dec 1, 2019

vdemeester commented Dec 2, 2019

afrittoli left a comment

afrittoli left a comment

afrittoli commented Dec 3, 2019

vdemeester left a comment

tekton-robot commented Dec 3, 2019

afrittoli left a comment

afrittoli left a comment

Store tektoncd/pipeline release version to a variable during build #1650

Store tektoncd/pipeline release version to a variable during build #1650

Conversation

waveywaves commented Nov 29, 2019

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Nov 29, 2019

waveywaves commented Nov 29, 2019

vdemeester left a comment

Choose a reason for hiding this comment

waveywaves commented Nov 29, 2019

vdemeester commented Nov 29, 2019

waveywaves commented Nov 29, 2019

waveywaves commented Nov 29, 2019

vdemeester commented Nov 29, 2019

waveywaves commented Nov 30, 2019 • edited Loading

chmouel commented Dec 1, 2019

vdemeester commented Dec 2, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli commented Dec 3, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Dec 3, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

waveywaves commented Nov 30, 2019 •

edited

Loading