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

Initial proposal for Console devfile feature w/o Build Guidance Spec #219

Merged
merged 6 commits into from
Dec 16, 2020

Conversation

maysunfaisal
Copy link
Member

Signed-off-by: Maysun J Faisal maysunaneek@gmail.com

What does this PR do?

This PR outlines a proposal for the OpenShift Console to consume build guidance within the boundaries of devfile spec 2.0.0 because the Build Guidance spec PR #127 is still an open discussion.

What issues does this PR fix or reference?

This proposal is for the POC PR openshift/console#6321

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

Docs PR


Console's devfile import POC is using a `BuildConfig` to build the Dockerfile from the dockerfile path location, currently found from the context dir. The POC PR `BuildConfig` outputs it to an `ImageStreamTag`. However, the POC PR assumes the `ImageStream` and `ImageStreamTag` are all the same name as the application name. This is tightly coupled and dependant on the information entered in the Console Devfile Import form and thus does not allow us to mention a custom image name in the devfile.

If we want to decouple `ImageStream` and `ImageStreamTag` from the application name, so that a custom image name can be specified in the devfile's component container; then we would need to update 1. image stream name 2. build config output image stream tag 3. deployment's container image in `pkg/server/devfile-handler.go` and also the annotation mapping for the `ImageStreamTag` in `getTriggerAnnotation()` in `frontend/packages/dev-console/src/components/import/import-submit-utils.ts`. This allows us to use the image name from devfile.yaml rather than always using the application name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Format the 3 points.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

### Note:
The context path is present in both the Console import form and the open devfile spec(Refer to the OpenShift Console Devfile Import screenshot above and the open Build Guidance PR screenshot below). Do we require two context dir information? Is the context dir in the Console form for devfile.yaml and the context dir in the build guidance spec for Dockerfile relative to devfile.yaml? Or do we always assume both the devfile.yaml and Dockerfile are always at the same dir level.

<img src="https://user-images.githubusercontent.com/31771087/99319306-4c19be80-2837-11eb-9639-a5c130deb4ba.png">
Copy link
Contributor

Choose a reason for hiding this comment

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

We should clarify that the first phase of the implementation will only support DockerFile, SourceToImage will not be implemented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

These devfile information would be parsed and returned by the library and thus ensuring a consistent UX.

### Note:
The above devfile proposal and POC PR assumes the `BuildConfig` outputs the build to an `ImageStreamTag` which is used by the OpenShift internal registry. To push the image to a non-OpenShift Image Registry, the `BuiidConfig` output can pushed to a private registry. OpenShift [doc](https://docs.openshift.com/container-platform/4.6/builds/managing-build-output.html) outlines how this can be achieved via a `BuildConfig` and pushes the build to a private or dockerhub registry. Pushing to a private registry requires secret configuration from the Docker config, and this OpenShift [doc](https://docs.openshift.com/container-platform/3.11/dev_guide/builds/build_inputs.html#using-docker-credentials-for-private-registries) explains how to achieve it.
Copy link
Contributor

Choose a reason for hiding this comment

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

To scope the work on the first phase, we should probably clarify that we are going to implement the ImageStream path only. We are not going to implement the private or dockerhub registry on the first phase of the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Copy link
Contributor

@elsony elsony 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-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elsony, johnmcollier, maysunfaisal
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

@maysunfaisal maysunfaisal merged commit 5307cdf into devfile:master Dec 16, 2020
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.

4 participants