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

Stack vs sample proposal and outer loop proposal #492

Merged
merged 5 commits into from
Jun 17, 2021
Merged

Conversation

elsony
Copy link
Contributor

@elsony elsony commented Jun 11, 2021

Signed-off-by: eyuen eyuen@redhat.com

What does this PR do?

Move design doc from gdoc (https://docs.google.com/document/d/1tbzZK9aWtpw5AtoZmUl1Jy8x8w9N5hhjXu3c26VYiYY/edit?usp=sharing and https://docs.google.com/document/d/1SL3ZJyAy8lp973EzQpY36VjTJHoyGXl2zUXokPiXwK8/edit?usp=sharing) to design doc in repo for public access.

What issues does this PR fix or reference?

Design doc for #279 and #51

Is your PR tested? Consider putting some instruction how to test your changes

Design doc changes only

Docs PR

Design doc changes only

Note: I accidentally closed the original PR #474 so this new one replaces it with all the review comments addressed.

Signed-off-by: eyuen <eyuen@redhat.com>
Signed-off-by: eyuen <eyuen@redhat.com>
Signed-off-by: eyuen <eyuen@redhat.com>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

I'm concerned we're providing too many options at once and overloading our definitions. It looks like the image component type has a lot of fields that have specific use cases, which may be hard to use/document.

From what I can tell, the full spec for image components is the below

image:
  imageName: {{myimage}}
  # Exclusive of s2i type
  dockerfile:
    # Common fields
    args: [ "arg1", "arg2", "arg3" ]
    envFrom:
    - secretRef:
        name: my-secret

    # Option 1
    buildContext: ${PROJECTS_ROOT}/build
    location: Dockerfile
    rootRequired: false

    # Option 2
    id: mycompany/my-node-stack-dockerfile/v2.2.2

    # Option 3
    git:
      remotes:
        origin: "https://github.com/odo-devfiles/nodejs-ex.git"
      location: Dockerfile
    
  # Exclusive of dockerfile type
  s2i:
    builderImageNamespace: mynamespace
    builderImageStreamTag: mytag
    scriptLocation:
      remotes:
        origin: "https://github.com/odo-devfiles/nodejs-ex.git"

We would need a very complicated set of validations here, e.g. it's either dockerfile or s2i, then if it's dockerfile, it's either {buildContext, location, rootRequired}, {id}, or {git, location}

Comment on lines 10 to 13
- Input\
The build may use different technologies for building, e.g. dockerfile, buildpacks
- Output:\
A container image that contains the microservice and ready to be deployed to Kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is more readable on single lines

    - **Input**: The build may use different technologies for building, e.g. dockerfile, buildpacks
    - **Output**: A container image that contains the microservice and ready to be deployed to Kubernetes

Also, - Input is missing a colon.

Copy link
Contributor Author

@elsony elsony Jun 17, 2021

Choose a reason for hiding this comment

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

I find using it more readable using separate lines so I keep the separate lines. I've added the missing colon.


We will cover two main stages:
1. Build the image
2. Deployment of the image. Deploy the image to a cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
2. Deployment of the image. Deploy the image to a cluster
2. Deploy the image to a cluster

Copy link
Contributor Author

@elsony elsony Jun 17, 2021

Choose a reason for hiding this comment

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

Fixed

Comment on lines 85 to 89
### Example using `apply` command on an image component:
commands:
- id: deploybuild
apply:
component: mydockerfileimage
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the purpose of the apply command here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the note section. It is used for controlling the image startup.

Comment on lines +98 to +100
dockerfile:
id: mycompany/my-node-stack-dockerfile/v2.2.2
args: [ ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this refers to. What is this an id into?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for referring to a resource within the registry.

Comment on lines +117 to +121
git:
remotes:
origin: "https://github.com/odo-devfiles/nodejs-ex.git"
location: Dockerfile
args: [ ]
Copy link
Contributor

Choose a reason for hiding this comment

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

We're really overloading .image.dockerfile with multiple disjoint sets of fields. This might be hard to use/understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is supposed to convert the existing design proposal spec so that everyone has access. If there are suggestions on spec changes, please open a separate item to discuss.

Comment on lines +123 to +125
The git definition will be the same as the one in `starterProjects` definition that supports `checkoutFrom`

`location`: the location of the dockerfile within the repo. If `checkoutFrom` is being used, the location will be relative to the root of the resources after cloning the resources using the `checkoutFrom` setting.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using a lot of logic from projects, why don't we just use projects for this? If you have the section above as a project in your devfile, you should just be able to use the first case (dockerfile + buildContext, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are only reusing the location definition, e.g. git and zip, of a project. Project has special meaning for defining and it is incorrect to retry to replace the dockerfile component with the project component.

1. Operators
1. Helm

The initial design will focus on the first two items for discussion. Other ways of building the image will be covered later.
Copy link
Contributor

Choose a reason for hiding this comment

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

These items may need to be reordered -- it looks like the proposal below covers deployment manifest and helm.

Copy link
Contributor Author

@elsony elsony Jun 17, 2021

Choose a reason for hiding this comment

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

Reordered the example to match.


## Overview

The existing devfile registry focuses on the stack support to provide generic language/framework/runtime, e.g. Node, Java maven, Quarkus, etc., support to build and run user applications. These devfiles are called ***stack devfiles***. There is another kind of devfile in a devfile registry that is tailored for a building and running a specific application. These devfiles are referred to as ___*sample devfiles*___ (example: https://github.com/redhat-developer/devfile-sample).
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, we should use either ***text*** or ___*text*___ for bold-italics.

Copy link
Contributor Author

@elsony elsony Jun 17, 2021

Choose a reason for hiding this comment

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

Fixed

Comment on lines +40 to +50
variables:
myimage: myimagename
components:
- name: mydockerfileimage
image:
imageName: {{myimage}}
dockerfile:
buildContext: ${PROJECTS_ROOT}/build
location: Dockerfile
args: [ "arg1", "arg2", "arg3" ]
rootRequired: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Use regular code blocks to get syntax highlighting.

Suggested change
variables:
myimage: myimagename
components:
- name: mydockerfileimage
image:
imageName: {{myimage}}
dockerfile:
buildContext: ${PROJECTS_ROOT}/build
location: Dockerfile
args: [ "arg1", "arg2", "arg3" ]
rootRequired: false
```yaml
variables:
myimage: myimagename
components:
- name: mydockerfileimage
image:
imageName: {{myimage}}
dockerfile:
buildContext: ${PROJECTS_ROOT}/build
location: Dockerfile
args: [ "arg1", "arg2", "arg3" ]
rootRequired: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +66 to +79
variables:
myimage: myimagename
components:
- name: mydockerfileimage
image:
imageName: {{myimage}}
dockerfile:
buildContext: ${PROJECTS_ROOT}/build
location: https://github.com/redhat-developer/devfile-sample/blob/master/src/Dockerfile
args: [ "arg1", "arg2", "arg3" ]
rootRequired: false
envFrom:
- secretRef:
name: my-secret
Copy link
Contributor

Choose a reason for hiding this comment

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

Enclose in ```yaml (....) ```

Applies to other code blocks in this document as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed all

@openshift-ci openshift-ci bot removed the lgtm label Jun 17, 2021
Signed-off-by: eyuen <eyuen@redhat.com>
@elsony
Copy link
Contributor Author

elsony commented Jun 17, 2021

I'm concerned we're providing too many options at once and overloading our definitions. It looks like the image component type has a lot of fields that have specific use cases, which may be hard to use/document.

@amisevsk This PR is supposed to just put the existing design proposal spec in the repo so that everyone has access. The specification has been gone through a couple of rounds of discussions in the cabal call. Therefore, I am only addressing formatting issues in this PR. If there are suggestions on spec changes, please open a separate item to discuss and bring it up to the cabal call for spec change discussion.

@openshift-ci
Copy link

openshift-ci bot commented Jun 17, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, elsony, GeekArthur

The full list of commands accepted by this bot can be found 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

@elsony elsony merged commit eb2947e into devfile:main Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants