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

SHIP-0021: Local Source Upload #11

Merged

Conversation

otaviof
Copy link
Member

@otaviof otaviof commented Jun 23, 2021

Local Source Upload (via STDIN)

The following video shows the contents of this enhancement-proposal in practice:

Live feature demo

R&D

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021
@openshift-ci openshift-ci bot requested review from qu1queee and sbose78 June 23, 2021 06:36
@otaviof otaviof force-pushed the local-source-upload-ep branch 2 times, most recently from 5cb91f5 to b5a81d1 Compare June 23, 2021 07:01
@otaviof
Copy link
Member Author

otaviof commented Jun 23, 2021

/retitle Local Source Upload (EP)

@openshift-ci openshift-ci bot changed the title [WIP] Local Source Upload (EP) Local Source Upload (EP) Jun 23, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021
@qu1queee qu1queee self-assigned this Jun 23, 2021
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

@otaviof please update this EP so it conforms to the SHIP template. This ensures consistency across all of our proposals and will speed up the review process.

Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

some initial relatively minor suggestions @otaviof but exciting stuff

ships/0005-local-source-upload.md Outdated Show resolved Hide resolved
With the data upload complete, the build-pod can continue to execute the build-strategy chosen,
producing a container-image at the end.

![source-upload sequence diagram](assets/0005-source-upload-sequence-diagram.png)
Copy link
Member

Choose a reason for hiding this comment

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


- Improve developer experience, local clone can produce a container-image using Shipwright machinery
- Move Shipwright CLI closer to developer's inner-loop
- Work as an alternative to [upload-to-container-registry][local-source-code-ep], in near future
Copy link
Member

Choose a reason for hiding this comment

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

did you mean to refer to https://github.com/shipwright-io/build/blob/main/docs/proposals/enable-local-source-code-support.md ?

fwiw, I'm not sure if I see it captured there, but I do recall community discussions where we wanted the source upload mechanism to be pluggable.

At least for me, I believe this EP is in the spirit of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the idea is to link with the EP you mentioned, and this paragraph could use better wording to reflect the upstream EP. I'll amend it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please consider the latest changes.

ships/0005-local-source-upload.md Outdated Show resolved Hide resolved
ships/0005-local-source-upload.md Outdated Show resolved Hide resolved
@otaviof
Copy link
Member Author

otaviof commented Jul 2, 2021

/retitle SHIP-0005: Local Source Upload

@openshift-ci openshift-ci bot changed the title Local Source Upload (EP) SHIP-0005: Local Source Upload Jul 2, 2021
Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

Just one more suggestion with this pass @otaviof


The approach proposed here is complementary to existing [EP][LocalSourceCodeEP] detailing local
source upload to a container-registry and later on self-extracting the image before the actual
container build process starts.
Copy link
Member

Choose a reason for hiding this comment

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

Ok after re-reading your updates and your EP refs, there are in fact connections to two existing EP's. LocalSourceCodeEP and RemoteArtifactsEP. That latter in fact ties into your API example. I think the former pertains to the notion of being able to swap different "source upload adapters" (i.e. kubectl cp vs. the registry approach).

Assuming I'm close here, I feel like a statement or two giving a high level description somewhere along these lines (or different details if you feel my summaries and somewhat off), explaining the relationship of your new EP here, with these other 2, is a good way to kick things off.

Then below the more detailed refs to these 2 EPs that you already have build upon the "introduction" I'm asking for here.

Copy link
Member

Choose a reason for hiding this comment

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

ok @otaviof pointed me to where below when he discusses the CLI element of the flow, the user will have a choice on which upload option to use.

I'm fine with capturing the "swapability" of all this there and just citing the "complementary" element with this EP to the other EP here at the beginning.

Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

Nice SHIP 🚀🚀🚀🚀🚀!, thanks for the effort you put here. I added a couple of comments I consider important to resolve prior to the merging.

Additionally, the proposal structure is missing some key points that are important to have, these are:

  • Test Plan ( under Proposal )
  • Release Criteria ( under Proposal )
  • Risks and Mitigations ( under Proposal )
  • Drawbacks
  • Alternatives
  • Implementation History

The Build Controller will inspect for a type `LocalCopy` on the "`sources"` slice, and therefore
instead of adding the git clone step, it will instead wait for the user to upload.

The `sources` slice is part of [Remote Artifacts (EP)][RemoteArtifactsEP] and has been worked on, so
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to put in that EP ( remote artifacts ) the final state of sources before we merge this one. As this is introducing a new type LocalCopy. I´m adding this to the next community meeting, see #15 (comment) .

The Remote Artifacts EP requires at least the following enhancements:

  • proposal on the bundle type (local source code EP)
  • proposal on the LocalCopy type (this EP)

Copy link
Member Author

@otaviof otaviof Jul 21, 2021

Choose a reason for hiding this comment

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

Sure! I understand the mix with Remote Artifacts due the initial text in this enhancement-proposal, so I'm now updating it to allow both implementations to happen independently. Here we have a API suggestion for when the Remote Artifact EP implementation comes along, and a additional method to not require API changes at all.

On amending the document I'm inclined to annotations instead of a full blown API change. Let me know your thoughts.

ships/0005-local-source-upload.md Outdated Show resolved Hide resolved
ships/0005-local-source-upload.md Outdated Show resolved Hide resolved
- Complementary to [upload-to-container-registry (EP)][LocalSourceCodeEP], uploading data directly to
the cluster instead

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to include under the Proposal section, how the usage of this feature will look in SHP cli. Keep in mind the following:

  • This provides another plugable option to achieve local source code. The different flavors of local source code should be almost transparent for the user. See for example the idea for the initial local source code option in ep.
  • We should avoid having different subcommands for it, so we might wanna signalize the flavors of local source code under the shp build create.

The above should be stated in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. I'll add on the document itself, my suggestion is based on the steps you see on the demo-video.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just added the command-line usage example in the proposal itself, taking in consideration we have two different upload mechanisms, so we need a flag to differentiate them. Please consider.

instance checksum, ownership (UID/GID) and more. Another important duty is to have `tar` installed
and compatible with the activities performed by the CLI.

The implementation written for research and development can be found at [otaviof/waiter][Waiter]
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that for waiting we will just prepend a new step, that will replace the git one when this feature is desired via CLI.

For the waiter image, we will need to place the code under https://github.com/shipwright-io/build/tree/main/cmd, similar to the git dir there, so that we can build an image from it, and define test cases under it´s package. This image should be compliant with how we are building images today ( ubi based ), so we need to be able to define this in our controller YAML with the ko nomenclature.

The waiter image should also support the definition of environment variables to modify the container spec where it is used, see our docs on configuration , in specific the GIT_CONTAINER_TEMPLATE.

All of the above needs to be mentioned here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads up. I've just amended the text to reflect those points, please consider.

2021/06/01 08:25:27 Done!
```

When the data transfer is complete, the CLI will execute "`waiter done`", via the same "`kubectl
Copy link
Contributor

Choose a reason for hiding this comment

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

have you contemplated the idea on what types of results can be surfaced when the wait is completed. See some examples of what I mean:

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting pointers, thank you. I've added wording to describe the Tekton's TaskResult for the Waiter, and also termination-log to inform detailed execution results/logs back to Tekton/Kubernetes. Please consider.

@otaviof otaviof force-pushed the local-source-upload-ep branch 6 times, most recently from 668fff9 to 7140b7c Compare July 22, 2021 10:23
@otaviof
Copy link
Member Author

otaviof commented Jul 22, 2021

Nice SHIP 🚀🚀🚀🚀🚀!, thanks for the effort you put here. I added a couple of comments I consider important to resolve prior to the merging.

Additionally, the proposal structure is missing some key points that are important to have, these are:
* Test Plan ( under Proposal )
* Release Criteria ( under Proposal )
* Risks and Mitigations ( under Proposal )
* Drawbacks
* Alternatives
* Implementation History

Thanks for the review. I've added the missing sections and went through all comments, so please consider the latest changes.

@gabemontero
Copy link
Member

I'm going to add the

/approve

as my comments have been addressed and based on the interaction between @otaviof and @qu1queee we are close

and let @otaviof and @qu1queee sort through the remaining details and then @qu1queee can lgtm when he is satisfied.

thanks

@gabemontero
Copy link
Member

duh I keep forgetting I'm not an approver.

So flipping, I'll

/lgtm

to provide the cross team sign off

then let @qu1queee approve :-)

@SaschaSchwarze0
Copy link
Member

I think we are near the finish line now.

Two remaining items:

(1) When there is a build and buildrun without any sources, what is the expected behavior? My expectation is that it simply retrieves no sources. The use case I have in mind is a build strategy that rebases an image (be it crane rebase or pack rebase, inputs (like which new base image) would be through params. It should not go into the local source code path. The following therefore must not be true:

To conclude my arguments, declaring a new type: LocalCopy only makes the API more complex, the desired result is, in practice, the same than .spec.source(s) without a Git repository.

(2) Source type vs annotation. I personally would like to omit annotations as much as possible (and our existing build-run-deletion is in my opinion already one we should not have introduced). So, my view on them:

  • Annotations can be used for alpha features for a product that is otherwise already beyond that. This allows to test new features without adding them to the API definition. That's what Kubernetes is doing. The SecComp profiles used to be annotations on the pod and are now first-class in the securityContext. See https://kubernetes.io/docs/tutorials/clusters/seccomp/.
  • Annotations must be used when product B enriches features of product A. An example: Kubernetes provides the generic Ingress resource. An implementation, the nginx-ingress controller supports vendor specific extensions through annotations.

In this scenario here, it is all about Shipwright Build. We define the resources and their structure and behavior. And we are still alpha. We have no need to force users to use annotations for features that can also be directly in the API object. That's more user-friendly I think. Users can explore the feature using kubectl explain. Users cannot misspell annotations (I know that this one will likely always be set by a CLI client). Value format like the duration can be defined in the OpenAPI schema.

I read your arguments but have to say that they do not convince me. The Shipwright CLI has a command line tool and you provide their flags to commands and they also go into the spec of the resource and not necessarily into annotations. And more or less all the information in the spec of the resource is relevant for the BuildRun execution (to define how it should behave) and after it (as reference information what the BuildRun did).

So, I won't block this at the end because of annotation vs. first class field, but would be interested in other's view on that aspect. @sbose78 @adambkaplan @gabemontero @HeavyWombat

BTW: One could even turn around "product B enriches features of product A" in that way: close this ship, implement a webhook that admits TaskRuns, loads their parent BuildRun, finds the annotations and hooks in the magic waiting step and wipes out all other source related steps. No worries, this is not meant serious. This feature is way too important so that I want it to be part of the core, including its API.

@otaviof
Copy link
Member Author

otaviof commented Sep 29, 2021

(1) When there is a build and buildrun without any sources, what is the expected behavior? My expectation is that it simply retrieves no sources.

Correct, that's the behavior I expect too.

The use case I have in mind is a build strategy that rebases an image (be it crane rebase or pack rebase, inputs (like which new base image) would be through params. It should not go into the local source code path. The following therefore must not be true:

To conclude my arguments, declaring a new type: LocalCopy only makes the API more complex, the desired result is, in practice, the same than .spec.source(s) without a Git repository.

How does the params are related to an .spec.source without an GIt repository declared? If I understand the rebase strategy correctly, in this case we would not allow any local source upload.

So, why do we need LocalCopy here after all? In my view, it's one more use-case where adding the type: LocalCopy or not having a Git repository in .spec.source(s), has the same outcome in practice.

(2) Source type vs annotation. I personally would like to omit annotations as much as possible (and our existing build-run-deletion is in my opinion already one we should not have introduced). So, my view on them:

In this scenario here, it is all about Shipwright Build. We define the resources and their structure and behavior. And we are still alpha. We have no need to force users to use annotations for features that can also be directly in the API object. That's more user-friendly I think. Users can explore the feature using kubectl explain. Users cannot misspell annotations (I know that this one will likely always be set by a CLI client). Value format like the duration can be defined in the OpenAPI schema.

For this specific use case we should always assume the user will be uploading source-code via our CLI, or some other type of automation that will handle the necessary complexity to achieve this task. So, let's assume it's a specific use-case, not the whole broad annotations vs. something else discussion, please.

I read your arguments but have to say that they do not convince me. The Shipwright CLI has a command line tool and you provide their flags to commands and they also go into the spec of the resource and not necessarily into annotations. And more or less all the information in the spec of the resource is relevant for the BuildRun execution (to define how it should behave) and after it (as reference information what the BuildRun did).

Well, so we are both not convinced :-) I think the API discussion should take a different forum than this EP, it only clutters the discussion with things that, as you say, are still in alpha phase. So, we could very well be working on the feature, and then ironing-out the API in parallel. I've went though the implementation, and it does not require API changes to do what we need.

And, sure, I'm not trying to "block" things as well, I'd like to discuss a concern before we move forward.

BTW: One could even turn around "product B enriches features of product A" in that way: close this ship, implement a webhook that admits TaskRuns, loads their parent BuildRun, finds the annotations and hooks in the magic waiting step and wipes out all other source related steps. No worries, this is not meant serious. This feature is way too important so that I want it to be part of the core, including its API.

All annotations mentioned here are meant for the BuildRun object only! I think it's worth to highlight this part, only on the BuildRun level we are interested to upload data, and only during data upload we need to tweak the timeout. As you may notice, is something particular to that resource, should not leak.

It's utterly relevant we let the Build resource out of this equation, since only during the actual build execution, in other words, something driven by a BuildRun instance, that we need to be concerned with waiting for upload. I think this bit is quite important to have a concise argument.

@gabemontero
Copy link
Member

gabemontero commented Sep 29, 2021

I think we are near the finish line now.

Two remaining items:

(1) When there is a build and buildrun without any sources, what is the expected behavior? My expectation is that it simply retrieves no sources. The use case I have in mind is a build strategy that rebases an image (be it crane rebase or pack rebase, inputs (like which new base image) would be through params. It should not go into the local source code path. The following therefore must not be true:

hey @SaschaSchwarze0 - so I believe I understand the scenario you've noted ^^ because we do have something similar with OpenShift Builds. But from an api object perspective, there isn't a precise 1 to 1 correspondence between openshift build and shipwright, so I'd like to make a couple of cross references with the current shipwright API, to make sure I understand you.

  1. So by "without any sources", I believe you mean both the old source field and newer, longer term, sources field, i.e. https://github.com/shipwright-io/build/blob/54859ced72e8ecc25174ab5d8496dcdfac33af12/pkg/apis/build/v1alpha1/build_types.go#L72-L78 are empty / nil.

am I right so far?

(fwiw there are analogous fields in the openshift build api)

  1. by "rebasing the image", I think you mean the user has specified a builder image at https://github.com/shipwright-io/build/blob/54859ced72e8ecc25174ab5d8496dcdfac33af12/pkg/apis/build/v1alpha1/build_types.go#L88 and you want to "rebuild" that but with param or env vars in place and setting of those items in the BuildRun that lead to modifications of the outputted image when compared to the builder image.

is that close to what you are referring to?

(fwiw there is an analogous notion in openshift build api as well here)

  1. if we are all good then, then yes, I agree we should allow such a scenario ... if you mean something else, could you perhaps provide some yaml snippets to illustrate what you are after, and help me visualize it?

And then I can see about circling whatever you have back to the context of what @otaviof has here.

I'll stop there though before attempting to cross reference with some of @otaviof 's points. Also, @otaviof @adambkaplan and I talked in a team meeting this morning and as I understand some updates from him on this EP / SHIP are coming. Those might help with these final points here.

@gabemontero
Copy link
Member

gabemontero commented Sep 29, 2021

So, I won't block this at the end because of annotation vs. first class field, but would be interested in other's view on that aspect. @sbose78 @adambkaplan @gabemontero @HeavyWombat

so in the conversation with @adambkaplan @otaviof and myself I just referenced in #11 (comment) I believe (@adambkaplan and @otaviof will of course chime in if they left with a different understanding) some of the updates (of the minor / clarifying variety) will bolster some of the yaml examples and more specific lists of annotations will make it clear that any annotations will not be used for what should be first class API fields (which is what I believe @otaviof also tried to convey with #11 (comment)), but just the deeper tuning variety sort of things or alpha / experimental things.

BTW: One could even turn around "product B enriches features of product A" in that way: close this ship, implement a webhook that admits TaskRuns, loads their parent BuildRun, finds the annotations and hooks in the magic waiting step and wipes out all other source related steps. No worries, this is not meant serious. This feature is way too important so that I want it to be part of the core, including its API.

@otaviof
Copy link
Member Author

otaviof commented Sep 30, 2021

I'll stop there though before attempting to cross reference with some of @otaviof 's points. Also, @otaviof @adambkaplan and I talked in a team meeting this morning and as I understand some updates from him on this EP / SHIP are coming. Those might help with these final points here.

Thanks for the conversation, folks. I reflected the points back to the EP text, and mainly those are:

  • Only the BuildRun resource will drive the processes described on the EP
  • Annotations are my recommendation for this feature
  • In light of the inputs, there's also a LocalCopy suggestion, which holds the attributes used directly on the resource .spec (as we call "statically typed")

I hope we've also cleared out the "rebase strategy" questions, but let me know if there's more to it.

/cc @SaschaSchwarze0 @gabemontero

Copy link
Member

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

ok @otaviof fwiw I think this pass that I've made is the most focused one I've been able to do, without distractions

while there are a lot of individual comments here, in total, in my opinion at least, the majority are minor in that they are of the organizational variety.

The high level items perhaps beyond minor:

  • I think we go with a combination of type: LocalCopy and annotations
  • there are some future work items I agree we don't have to take on with the PR, this pass on things, but some acknowledgement of them for subsequent possible consideration is warranted.

Proposal to improve developer-experience by allowing to upload a local repository clone to the
Kubernetes cluster, and use Shipwright Build Controller to produce a container-image out of it.

As a developer, I can try Shipwright before opening a pull-request, and take advantage of the cluster
Copy link
Member

Choose a reason for hiding this comment

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

This may have changed since this SHIP was created back in June, but the ship enhancement proposal template has a User Stories section at the top of the Proposal section

This paragraph feels very close to a user story description.

I would suggest creating a user story section per the latest template, and moving this paragraph there, we perhaps some minor modification, to make it a user story description.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I'm renaming and reviewing the section formerly named "Use-Cases" to become the "User Stores", please consider latest changes. I'm also rewording the summary to reflect section, it should introduce the reader to what will be explained during the EP.

ships/0021-local-source-upload.md Outdated Show resolved Hide resolved
ships/0021-local-source-upload.md Show resolved Hide resolved
Comment on lines +61 to +74
The user-experience using the CLI should be as simple as the following example, assuming the user is
present at the same directory than the source code, and a `Build` named `nodejs-ex` exists.

```bash
shp buildrun upload nodejs-ex --to-cluster
```

Here, a new sub-command, `upload` is introduced and the user may choose `--to-cluster` or
`--to-registry` options, to respectively, upload to the cluster directly or upload to
container-registry.
Copy link
Member

Choose a reason for hiding this comment

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

I could see what @otaviof has here as an initial stake in the ground that may be adjusted in a few ways, including what @SaschaSchwarze0 has articulated, but certainly not limitied to. As such, I think it is fair to let the fine tuning of the options get sorted out during the actual PR and early consumption by users, and adjust / add options based on feedback.

And if need be, update this SHIP after its initial merge/introduction with those adjustments. Certainly we see such iterative EP / code / update EP patterns with KEPs, TEPs, openshift EPs.

In summary, IMO @otaviof continue to keep @SaschaSchwarze0 's points here in the back of your mind, leave this comment open / unresolved, take so we can easily come back to if user feedback warrants it, but conversely, @SaschaSchwarze0 let's not block merge on this point.

What does everyone think?

Comment on lines 93 to 98
On the CLI, it will be watching for the creation of pods using a `LabelSelector` _(3)_, which
narrows the search criteria to only find the pod triggered by step 1.
Copy link
Member

Choose a reason for hiding this comment

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

cool ... then I think @otaviof you should call out the existing label explicitly in your text here

Only the `BuildRun` object will accept the "flags" to trigger the waiting for user upload, meaning
this type of approach is not meant to leak towards the parent `Build` resource.

##### Alternative BuildRun API
Copy link
Member

Choose a reason for hiding this comment

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

not sure about the word "Alternative" here

I was envisioning that we at least had the type: LocalCopy from the outset, but then initially control the other implementation detail items via annotation.

Copy link
Member

Choose a reason for hiding this comment

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

I also think we need to mention either here or somewhere that if we are not doing it at least initially, we need to start thinking about whether opting into LocalCopy moves from the BuildRun to the Build .... "promoting" it if you will.

given the uncertainty we have on several aspects of the implementation details (file ownership, upload method, etc), I like starting off at the BuildRun ... feels safer. Even if we add the field to the Build later, the BuildRun field could still serve as an override.

But conversely, I think it is likely that when we start getting feedback from users, they are going to at least ask about the selection for local source in the sources array (where the URL in BuildSource could be file:// ....

So again, even if we are not going there initially with this EP in its current form, so verbiage about keeping an eye toward ^^ seems warranted IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Seperate point: yes, with the need to "opt in" via type: LocalCopy and/or annotations, at least IMO, we have preserved the rebase image scenario from @SaschaSchwarze0 as I understand it).

Copy link
Member

Choose a reason for hiding this comment

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

another clarification occurred to me

  • ideally, we end up in a place like most k8s api where there are just api fields and we don't need annotations to control
  • if there is enough uncertainty, of suspicion that the API might have to pivot, an interim combination of new api fields and annotations exist
  • so we are just iterating on what the initial balance of new api and annotations is
  • with that, at least for me, type: LocalCopy with a combination of initial annotations is the correct initial balance

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure about the word "Alternative" here

I was envisioning that we at least had the type: LocalCopy from the outset, but then initially control the other implementation detail items via annotation.

I'm rewording it to explain the comparison between those two approaches instead. Please consider.

timeout: 30s
```

As a alternative to use annotations described before, a new source type `LocalCopy` will be
Copy link
Member

Choose a reason for hiding this comment

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

yeah again ... I see it as in combination .... have LocalCopy type, plus annoatations to control some of the items we want feedback on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm rewording this part to explain the tradeoffs of each approach. They could be used in combination, but since the express the same fields, I think we should pick one.

Copy link
Member

Choose a reason for hiding this comment

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

ok this to me is an either / or expression ... either all annotations or all api fields

those are certainly valid options, but I at least think a limited subset of api fields (i.e. just the type) followed by annotations is also viable

but between the two you have listed here, if nobody like the mix of annotations and a small subset of api fields like me, then I vote for explicit API fields, and not annotations. We just then take a conservative approach and minimize the api fields, assuming reasonable defaults for things like timeouts, and get user feedback, which probably leads to use adding things we are thinking about (like timeout) and others we are not.

but to be clear I like

---
apiVersion: shipwright.io/v1alpha1
kind: BuildRun
metadata:
  annotations:
    buildrun.shipwright.io/upload.wait: "true"
    buildrun.shipwright.io/upload.wait.timeout: "30s"
spec:
  sources:
    - name: src
      type: LocalCopy

where we later on add buildrun.shipwright.io/upload.type: when we have the protoype of something to choose from ... then, when we get confidence in our mutliple choices, we decide how to make that annotation a new field

and if timeout is something that varies widely, we make that an api field ... but if the default we pick works in enough cases, we leave the tuning know as an annotation


Additionally, the `timeout` attribute sets how long the `waiter` will hold back for user upload data.

#### Waiting Mechanism
Copy link
Member

Choose a reason for hiding this comment

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

were you planning on describing the "kubectl cp" related waiting mechanism if you will as part of a potential separate PR / effort to this EP @otaviof ?

if so that is fine with me ... just wanted to make sure

Copy link
Member

Choose a reason for hiding this comment

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

or if you think a waiter is not needed for kubectl cp that is fine too ... in that case, I suppose that comes up then when you formally get into kubectl cp

Copy link
Member Author

@otaviof otaviof Oct 1, 2021

Choose a reason for hiding this comment

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

In fact kubectl cp only deals with streaming the data via the STDIN to a POD. It assumes the user will only use it against pod's containers that are ready.

In this EP we create the technical mechanism so our build-pod can wait for user input, and also, have the tooling needed for this task. For the record, we reuse kubectl cp foundations regarding stream data over STDIN.


### Use-Cases

#### Developer's Inner-Loop
Copy link
Member

Choose a reason for hiding this comment

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

per comments above when you mentioned this one, this is one of those user stories ... not too surprising given you "use-cases" section title above :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make sure both places are consolidated, and the names match the template as well.

Copy link
Member

Choose a reason for hiding this comment

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

ok ... I see you have not done it yet, but based on your comment ^^ I'm assuming this will be moved up to the user stories here will be moved up to the Proposal section with your next iteration

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, the section has been renamed to "User Stories", and it's inside the "Proposal" section already. I was not planing to move it all the way up to the top, tbh. Do you think we should discuss the user-stores before discussing the proposal itself?

Copy link
Member

Choose a reason for hiding this comment

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

moving this section is what the current SHIP template calls for

If none of the other reviewers object, and you feel strongly about it, I'll consent to not blocking the merge of this PR on that point.


#### Continuous-Integration

During CI pipelines Shipwright Build-Controller can be employed to build container-images based on
Copy link
Member

Choose a reason for hiding this comment

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

this also feels like a user story to mention up top in a user story section per the latest template ... I don't remember you mentioning this one up top in the other sections, like you did with the "as a dev I want to try out locally first" one

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using the section as "user stories", as you suggested. Indeed, makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

OK to reword this whole section as as user story, here is my suggestions:

"As as user or maintainer of a CI system, I would like to be able to employ the building of container images from a local repository clone for various test scenarios further down in the CI pipeline. And in doing so, I would like to no require the installation of container managers like Docker to build those container images."

@gabemontero
Copy link
Member

I think we are near the finish line now.
Two remaining items:
(1) When there is a build and buildrun without any sources, what is the expected behavior? My expectation is that it simply retrieves no sources. The use case I have in mind is a build strategy that rebases an image (be it crane rebase or pack rebase, inputs (like which new base image) would be through params. It should not go into the local source code path. The following therefore must not be true:

hey @SaschaSchwarze0 - so I believe I understand the scenario you've noted ^^ because we do have something similar with OpenShift Builds. But from an api object perspective, there isn't a precise 1 to 1 correspondence between openshift build and shipwright, so I'd like to make a couple of cross references with the current shipwright API, to make sure I understand you.

1. So by "without any sources", I believe you mean both the old `source` field and newer, longer term, `sources` field, i.e. https://github.com/shipwright-io/build/blob/54859ced72e8ecc25174ab5d8496dcdfac33af12/pkg/apis/build/v1alpha1/build_types.go#L72-L78 are empty / nil.

am I right so far?

(fwiw there are analogous fields in the openshift build api)

1. by "rebasing the image", I **think** you mean the user has specified a builder image at https://github.com/shipwright-io/build/blob/54859ced72e8ecc25174ab5d8496dcdfac33af12/pkg/apis/build/v1alpha1/build_types.go#L88 and you want to "rebuild" that but with param or env vars in place and setting of those items in the BuildRun that lead to modifications of the outputted image when compared to the builder image.

Realized my link ^^ was a bit incomplete ... wasn't just line 88 (which is just the build tools), but 88 to 100 ie. https://github.com/shipwright-io/build/blob/main/pkg/apis/build/v1alpha1/build_types.go#L88-L100

The dockerfile ref captures the FROM image you are rebasing

the params note some of the tuning options

@otaviof otaviof force-pushed the local-source-upload-ep branch 4 times, most recently from 23ba113 to bddc9d1 Compare October 1, 2021 08:00

### Use-Cases

#### Developer's Inner-Loop
Copy link
Member

Choose a reason for hiding this comment

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

ok ... I see you have not done it yet, but based on your comment ^^ I'm assuming this will be moved up to the user stories here will be moved up to the Proposal section with your next iteration

ships/0021-local-source-upload.md Outdated Show resolved Hide resolved
ships/0021-local-source-upload.md Outdated Show resolved Hide resolved
ships/0021-local-source-upload.md Outdated Show resolved Hide resolved

#### Continuous-Integration

During CI pipelines Shipwright Build-Controller can be employed to build container-images based on
Copy link
Member

Choose a reason for hiding this comment

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

OK to reword this whole section as as user story, here is my suggestions:

"As as user or maintainer of a CI system, I would like to be able to employ the building of container images from a local repository clone for various test scenarios further down in the CI pipeline. And in doing so, I would like to no require the installation of container managers like Docker to build those container images."

In the `BuildRun` resource, the Build Controller will inspect for the following annotations:

- `build.shipwright.io/upload.wait`: boolean attribute, only `true` or `false` accepted;
- `build.shipwright.io/upload.wait.timeout`: sets the timeout;
Copy link
Member

Choose a reason for hiding this comment

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

ok cool ... I like the pattern of all the annotations starting with the same prefix

I do wonder if rather upload if it should be localSource ....we ultimately might have other "local source" tuning params beside things specific to the upload ..... language detection a la oc new-app comes to mind

you up for changing to that @otaviof ?

Co-authored-by: Enrique Encalada <encaladaenrique@gmail.com>
Co-authored-by: Adam Kaplan <adam.kaplan@redhat.com>
Co-authored-by: Gabe Montero <gmontero@redhat.com>
@otaviof
Copy link
Member Author

otaviof commented Oct 4, 2021

I do wonder if rather upload if it should be localSource ....we ultimately might have other "local source" tuning params beside things specific to the upload ..... language detection a la oc new-app comes to mind

@gabemontero, sure! I'm open for that. I've been thinking on it, and in summary, what we are discussing is how the Build Controler will populate the /workspace/source directory, right?

It boils down to three options:

  • Checkout: The Build Controller will checkout the Git repository data;
  • Upload: The Build Controller will wait for user direct data upload (via STDIN redirect);
  • Bundle-Image: The Build Controller will make use of the bundle imabe, plus auto-extration techniques;

What do you think about using Checkout (default), DirectUpload and BundleImage as the possible types? That is, for instance:

---
apiVersion: shipwright.io/v1alpha1
kind: BuildRun
metadata:
  annotations:
    buildrun.shipwright.io/workspaceSource.type: "DirectUpload"
    buildrun.shipwright.io/workspaceSource.timeout: "30s"

The Build Controller can rely on the type to decice how the TaskRun will be generated, either using the git clone or the waiter described on this EP. Initally on the waiter will need timeout setting, but we could consider also having it for the other two options.

What do you think?

@sbose78
Copy link
Member

sbose78 commented Oct 4, 2021

I think we could do this:

  • A first class API for specifying intent to have "local source" upload ( we know we want this for sure )
  • .. but have annotations to tune it. ( subject to change )

Let's get the PR up, I suspect we could all have a change of opinion based on how things look. As with any API contract, annotations or first class, we should think about how to phase out the annotations when we move a subset of them into first class APIs

@gabemontero
Copy link
Member

discussion on how to proceed in today's community meeting is summarized at #32 (comment)

@gabemontero
Copy link
Member

discussion on how to proceed in today's community meeting is summarized at #32 (comment)

@otaviof just wanted to make sure if I captured the next steps ^^ as you recall them as well ... thanks

@otaviof
Copy link
Member Author

otaviof commented Oct 6, 2021

discussion on how to proceed in today's community meeting is summarized at #32 (comment)

@otaviof just wanted to make sure if I captured the next steps ^^ as you recall them as well ... thanks

Thanks for taking notes, Gabe!

To explain it on my own words. I think we are more inclined to use a "statically typed" API; we would like to see the pull-requests open so we can have a better grasp of how this feature will look like, so we can discuss based on it; we may need to update this EP based on the initial implementation feedback.

@gabemontero
Copy link
Member

discussion on how to proceed in today's community meeting is summarized at #32 (comment)

@otaviof just wanted to make sure if I captured the next steps ^^ as you recall them as well ... thanks

Thanks for taking notes, Gabe!

To explain it on my own words. I think we are more inclined to use a "statically typed" API; we would like to see the pull-requests open so we can have a better grasp of how this feature will look like, so we can discuss based on it; we may need to update this EP based on the initial implementation feedback.

fwiw your words reconcile in my head with mine @otaviof !!

so if you can place those words in the EP, capturing this current sentiment, so this current state of things is documented, I'll tag this and we can merge it.

As I currently read things, you've listed the options (and for example statically typed is optional), but have not precisely captured the consensus we left Monday's meeting with.

Of course if you disagree, please point me to the text you think I am overlooking. Otherwise I'll wait for your new commit.

thanks!

Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2021
@openshift-merge-robot openshift-merge-robot merged commit e6e48ba into shipwright-io:main Oct 13, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants