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

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

Merged
merged 1 commit into from
Dec 4, 2019

Conversation

waveywaves
Copy link
Member

Changes

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

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

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

@googlebot 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 tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 29, 2019
@tekton-robot
Copy link
Collaborator

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.

@tekton-robot 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
Copy link
Member Author

cc @chmouel @vdemeester

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.

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
Copy link
Member Author

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

@vdemeester
Copy link
Member

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

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

@waveywaves
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member Author

waveywaves commented Nov 30, 2019

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
Copy link
Member

chmouel commented Dec 1, 2019

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
Copy link
Member

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

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@tekton-robot 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
tekton/release-pipeline.yaml Show resolved Hide resolved
.ko.yaml Outdated Show resolved Hide resolved
.ko.yaml.release Outdated Show resolved Hide resolved
@waveywaves waveywaves force-pushed the update/pipeline-version branch 4 times, most recently from c816ac7 to 3d5ebac Compare December 3, 2019 11:31
Copy link
Member

@afrittoli afrittoli 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 the updates. This looks good now!
Only one comment on the nightly task please.

tekton/publish-nightly.yaml Outdated Show resolved Hide resolved
@afrittoli
Copy link
Member

/ok-to-test

@tekton-robot 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 waveywaves force-pushed the update/pipeline-version branch 4 times, most recently from 4012daa to b5a13cb Compare December 3, 2019 12:26
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.

Looks good :)

tekton/publish-nightly.yaml Outdated Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

[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:

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 Dec 3, 2019
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
tekton/publish-nightly.yaml Outdated Show resolved Hide resolved
Fixes tektoncd#1543
Introduces variable for storing the pipeline version
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

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

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2019
@tekton-robot tekton-robot merged commit f85c53f into tektoncd:master Dec 4, 2019
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 17, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 17, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 17, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 20, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 20, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 20, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 20, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 20, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Dec 20, 2019
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Jan 3, 2020
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 <[email protected]>
waveywaves added a commit to waveywaves/tekton-pipeline that referenced this pull request Jan 6, 2020
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 <[email protected]>
tekton-robot pushed a commit that referenced this pull request Jan 6, 2020
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 <[email protected]>
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add version label to pipelines controller deployment
6 participants