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

As developer, I'd like to see build spec snapshot for each buildrun #151

Closed
ZhuangYuZY opened this issue Apr 22, 2020 · 12 comments
Closed

Comments

@ZhuangYuZY
Copy link

ZhuangYuZY commented Apr 22, 2020

So far the design is buildrun CR has buildRef property which refers a build CR. And developer could create multiple buildrun CRs based on one build CR. The problem is developer could update build CR to change configuration in anytime. That means if developer want to check a buildrun status, then developer could not get build information, as build CR may update by someone else already.

Proposal:

  1. create revision for build CR, then build run refer to build revision. -- Which is complicated from both end user(another concept revision need to learn) and development(more dev effort) perspective.
  2. When reconcile buildrun cr, take a snapshot for build cr, and store it in 1) buildrun cr annotation or 2) new property in buildrun status.

@sbose78 @zhangtbj your thought ?

@sbose78
Copy link
Member

sbose78 commented Apr 22, 2020

We should have this!
I was debating with myself it should go to the status section.

@otaviof
Copy link
Member

otaviof commented Apr 27, 2020

I also think that's good use-case to tackle. And, we can use kubectl.kubernetes.io/last-applied-configuration as inspiration to store previous configuration.

@zhangtbj
Copy link
Contributor

I had some discussions in kube community:
https://kubernetes.slack.com/archives/C09NXKJKA/p1588082143131400

I think it is also a good point:

  • Annotations are usually for consumers/users to write
  • And status for consumers/users to read

For this requirement, I also agree that annotation is better:

  • First, we just want to write and keep the build config in buildrun, just for future debug/review, not for read
  • Second, the build spec is stable and never be changed for this buildRun. The status should be used to show the current buildRun status, status change is an important point.

So I also suggest to use an annotation in buildrun like this:

---
apiVersion: build.dev/v1alpha1
kind: BuildRun
metadata:
  name: buildah-golang-buildrun
    annotations:
      build.build.dev/configuration: |
        {"apiVersion":"build.dev/v1alpha1","kind":"Build","metadata":{"annotations":{},"labels":{}, ....}
spec:
  buildRef:
    name: buildah-golang-build

And also save both build and buildrun as Annotations in generated Tekton taskrun.

@otaviof
Copy link
Member

otaviof commented May 4, 2020

@zhangtbj thanks! I think we have a good plan to implement this feature. :-)

@zhangtbj
Copy link
Contributor

zhangtbj commented May 6, 2020

Hi @sbose78 ,

BTW, do you also agree to use the annotation (we can define the name as build.build.dev/configuration) to store the build spec in buildRun? :)

@sbose78
Copy link
Member

sbose78 commented May 6, 2020

Yes!

@SaschaSchwarze0
Copy link
Member

BTW: Tekton just did the same for TaskSpec/TaskRun and stored the TaskSpec in TaskRun.Status.TaskSpec, see tektoncd/pipeline#2444

@zhangtbj
Copy link
Contributor

zhangtbj commented May 6, 2020

Hi @SaschaSchwarze0 , an interesting finding,

I am not sure why they choose to store the TaskSpec in the status, but Andrea and Simon are all come from IBM.

Can you please help ask them in PR or slack to know why they choose the store in the TaskRun.Status.TaskSpec ? (In my opinion, it is stable and never changed)

Thanks!

@sbose78
Copy link
Member

sbose78 commented May 6, 2020

Putting things in the status is a more strongly typed way of doing things.

I would image a Kubernetes or Openshift UI to easy render the corresponding Task that way.

Either way, it doesn't hurt us to begin with adding it in an annotation for now. API changes needs to slow and we'll thought out :)

@zhangtbj
Copy link
Contributor

zhangtbj commented May 7, 2020

Hi @sbose78 ,

Some news. :)

I talked with multiple teams and got some feedbacks, that they think status is better for this case:

  • Keep BuildSpec in annotation may be removed or changed by the client, status cannot be changed by end-user
  • Better showing for human, Having it in the status at least let's you use -o jsonpath with kubectl, which wouldn't work if it was just JSON in an annotation
  • Typically an annotation is an authored/generated part of a BuildRun CRD so Tekton guys think the status is the right place
  • Both of two ways are similiar storage cost/limits

I talked with @qu1queee and @SaschaSchwarze0 , we agreed to switch to Status way.

How about you? @sbose78 and @otaviof ? :)

Thanks all!

@sbose78
Copy link
Member

sbose78 commented May 11, 2020

Tekton guys think the status is the right place

Let's go for status to keep things uniform.

@qu1queee
Copy link
Contributor

This was done already, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants