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

Adding dockerfile component #106

Closed
wants to merge 3 commits into from
Closed

Conversation

ranakan19
Copy link

What does this PR do?

Adds dockerfile component for devfile 2.1.0

What issues does this PR fix or reference?

In reference to https://github.com/devfile/parser/issues/26

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

Not applicable.

Docs PR

Co-authored-by: William Tam <wtam@redhat.com>
@ranakan19 ranakan19 requested a review from wtam2018 August 5, 2020 11:54
@kadel
Copy link
Member

kadel commented Aug 5, 2020

This is targeted to 2.1.0 but we have only a single version of types. How is this versioning going to work?

@ranakan19
Copy link
Author

ranakan19 commented Aug 6, 2020

This is targeted to 2.1.0 but we have only a single version of types. How is this versioning going to work?

@kadel I'm not sure i understand your question, do you mean the schemaVersion? In the devfile library, devfile 2.0.0 and 2.1.0 are separate schema, but i figured that the kubernetes-api can have all the common structs to be used.

@kadel
Copy link
Member

kadel commented Aug 7, 2020

This is targeted to 2.1.0 but we have only a single version of types. How is this versioning going to work?

@kadel I'm not sure i understand your question, do you mean the schemaVersion? In the devfile library, devfile 2.0.0 and 2.1.0 are separate schema, but i figured that the kubernetes-api can have all the common structs to be used.

Yes, I'm wondering how is this going to work with schemaVersion.

For example, If I have a devfile that has schemaVersion: 2.0.0 with Dockerfile component.
Would that get parsed or not?

@kadel
Copy link
Member

kadel commented Aug 7, 2020

btw @ranakan19, you need to run ./docker-run.sh ./build.sh to update
Json Schema and other autogenerated files. Autogenerated files should be included in your PR as a separate commit.

@davidfestal
Copy link
Collaborator

This is targeted to 2.1.0 but we have only a single version of types. How is this versioning going to work?

@kadel I'm not sure i understand your question, do you mean the schemaVersion? In the devfile library, devfile 2.0.0 and 2.1.0 are separate schema, but i figured that the kubernetes-api can have all the common structs to be used.

Yes, I'm wondering how is this going to work with schemaVersion.

For example, If I have a devfile that has schemaVersion: 2.0.0 with Dockerfile component.
Would that get parsed or not?

I assume that at some point we would have to keep several versions of the K8S apis (like the v1, v2alpha1, etc, ...), and possibly maintain a mapping between a devfile schema version (semantic-versioning compatible) and the corresponding K8S API version.

That's something that is done for K8S itself for example. But we should obviously discuss how this could be achieved as simply as possible in our case, knowing that we want to avoid in upcoming devfile schema versions, changes that would introduce incompatible changes.

Copy link
Collaborator

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

In order to correctly manage:

  • unions
  • mandatory fields that should be made optional in overrides

There would be some additional changes to add to this PR to have it complete. I'll provide them as a PR on your PR asap.

NOTE: In the future things should be simpler since:

  • PR checks would validate the changes
  • Some tooling would allow generating these additional manual changes automatically.

@davidfestal
Copy link
Collaborator

I created PR ranakan19#2 to fix some small errors, and add required code for correct union management.

@davidfestal
Copy link
Collaborator

I have 2 other questions:

  1. In this code block:
//Source within dockerfile component
type Source struct {
	// path to local source directory folder
	SourceDir string `json:"sourceDir,omitempty"`

	// path to source repository hosted locally or on cloud
	Location string `json:"location,omitempty"`
}

Are the 2 fields mutually exclusive and can they both have a value defined.
If they are mutually exclusive, it could be defined as a union, so that the schema itself would ensure this union-member oneness.

  1. Does overriding the dockerimage component of a parent makes sense ?
    For now the dockerimage is not added into the overrides. And if yes, does it also make sense to override a dockerfile component that would be encapsulated in a plugin definition ?

Copy link
Collaborator

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

I assume we should first clearly define how devfile schema versioning vs K8S APIs versioning will work, before merging this PR.
cc @l0rd

@wtam2018
Copy link

wtam2018 commented Aug 11, 2020

Are the 2 fields mutually exclusive and can they both have a value defined.

Got some clarification ... SourceDir is the directory where the source is located in a git source repository (i.e. where is the source dir after source git repository is cloned.). So, SourceLocation and SourceDir are not mutually exclusive.

@ranakan19
Copy link
Author

I have 2 other questions:

  1. In this code block:
//Source within dockerfile component
type Source struct {
	// path to local source directory folder
	SourceDir string `json:"sourceDir,omitempty"`

	// path to source repository hosted locally or on cloud
	Location string `json:"location,omitempty"`
}

Are the 2 fields mutually exclusive and can they both have a value defined.
If they are mutually exclusive, it could be defined as a union, so that the schema itself would ensure this union-member oneness.

  1. Does overriding the dockerimage component of a parent makes sense ?
    For now the dockerimage is not added into the overrides. And if yes, does it also make sense to override a dockerfile component that would be encapsulated in a plugin definition ?

for 1, Yes the fields are mutually exclusive. I'm not sure about 2, we should discuss it in the meeting.

Copy link

@wtam2018 wtam2018 left a comment

Choose a reason for hiding this comment

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

This PR is superseded by #127. Please close this one.

@sleshchenko
Copy link
Member

Closing in favor of #127

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

Successfully merging this pull request may close these issues.

5 participants