-
Notifications
You must be signed in to change notification settings - Fork 15
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
SHIP-0021: Local Source Upload #11
Conversation
5cb91f5
to
b5a81d1
Compare
/retitle Local Source Upload (EP) |
There was a problem hiding this 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.
b5a81d1
to
9589a6a
Compare
There was a problem hiding this 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
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love the diagram :-)
a direct link for others to see it directly: https://github.com/shipwright-io/community/blob/9589a6a3c3407375bc3c044fad1ad5ce42bc7f2d/ships/assets/0005-source-upload-sequence-diagram.png
ships/0005-local-source-upload.md
Outdated
|
||
- 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
/retitle SHIP-0005: Local Source Upload |
9589a6a
to
2bc8088
Compare
There was a problem hiding this 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
ships/0005-local-source-upload.md
Outdated
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
ships/0005-local-source-upload.md
Outdated
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
- Complementary to [upload-to-container-registry (EP)][LocalSourceCodeEP], uploading data directly to | ||
the cluster instead | ||
|
||
## Proposal |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ships/0005-local-source-upload.md
Outdated
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ships/0005-local-source-upload.md
Outdated
2021/06/01 08:25:27 Done! | ||
``` | ||
|
||
When the data transfer is complete, the CLI will execute "`waiter done`", via the same "`kubectl |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
668fff9
to
7140b7c
Compare
Thanks for the review. I've added the missing sections and went through all comments, so please consider the latest changes. |
7140b7c
to
c7df581
Compare
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 :-) |
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
(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 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. |
Correct, that's the behavior I expect too.
How does the So, why do we need
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.
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.
All annotations mentioned here are meant for the It's utterly relevant we let the |
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.
am I right so far? (fwiw there are analogous fields in the openshift build api)
is that close to what you are referring to? (fwiw there is an analogous notion in openshift build api as well here)
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. |
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.
|
06194d7
to
c04290f
Compare
Thanks for the conversation, folks. I reflected the points back to the EP text, and mainly those are:
I hope we've also cleared out the "rebase strategy" questions, but let me know if there's more to it. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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?
ships/0021-local-source-upload.md
Outdated
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. |
There was a problem hiding this comment.
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
ships/0021-local-source-upload.md
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ships/0021-local-source-upload.md
Outdated
|
||
#### Continuous-Integration | ||
|
||
During CI pipelines Shipwright Build-Controller can be employed to build container-images based on |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
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 |
23ba113
to
bddc9d1
Compare
|
||
### Use-Cases | ||
|
||
#### Developer's Inner-Loop |
There was a problem hiding this comment.
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
|
||
#### Continuous-Integration | ||
|
||
During CI pipelines Shipwright Build-Controller can be employed to build container-images based on |
There was a problem hiding this comment.
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."
ships/0021-local-source-upload.md
Outdated
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; |
There was a problem hiding this comment.
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 ?
caac9a8
to
69bcddc
Compare
Co-authored-by: Enrique Encalada <[email protected]> Co-authored-by: Adam Kaplan <[email protected]> Co-authored-by: Gabe Montero <[email protected]>
69bcddc
to
a6b2fa8
Compare
@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 It boils down to three options:
What do you think about using ---
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 What do you think? |
I think we could do this:
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 |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Local Source Upload (via
STDIN
)The following video shows the contents of this enhancement-proposal in practice:
R&D