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 a devfile for the Universal Developer Image #177

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

ibuziuk
Copy link
Collaborator

@ibuziuk ibuziuk commented Apr 6, 2023

What does this PR do?:

Adding a devfile for the Universal Developer Image

Which issue(s) this PR fixes:

eclipse-che/che#22126

PR acceptance criteria:

  • Contributing guide
    Have you read the devfile registry contributing guide and followed its instructions?
  • Test automation
    Does this repository's tests pass with your changes?
  • Documentation
    Does any documentation need to be updated with your changes?
  • Check Tools Provider
    Have you tested the changes with existing tools, i.e. Odo, Che, Console? (See devfile registry contributing guide on how to test changes)

How to test changes / Special notes to the reviewer:

image

  • need to add an icon for the stack, can we potentially reuse devfile icon?

@openshift-ci
Copy link

openshift-ci bot commented Apr 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ibuziuk ibuziuk changed the title Adding dedicated devfile for the Universal Developer Image Adding a dedicated devfile for the Universal Developer Image Apr 6, 2023
@ibuziuk ibuziuk changed the title Adding a dedicated devfile for the Universal Developer Image Adding a devfile for the Universal Developer Image Apr 6, 2023
@ibuziuk ibuziuk requested a review from l0rd April 6, 2023 15:26
@openshift-ci openshift-ci bot added the lgtm Looks good to me label Apr 6, 2023
@openshift-ci
Copy link

openshift-ci bot commented Apr 6, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ibuziuk, l0rd
Once this PR has been reviewed and has the lgtm label, please assign elsony for approval. For more information see the Kubernetes Code Review Process.

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

@johnmcollier
Copy link
Member

build is failing due to the fact that metadata.projectType is not set. Not sure why the field is required though, according to the spec it is optional

The field isn't required in the spec, but the registry viewer requires it currently

@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Apr 7, 2023

@johnmcollier is there some generic value we can use for UDI? e.g. 'default' ?

@openshift-ci openshift-ci bot removed the lgtm Looks good to me label Apr 7, 2023
@ibuziuk ibuziuk marked this pull request as ready for review April 7, 2023 11:24
stacks/udi/devfile.yaml Outdated Show resolved Hide resolved
stacks/udi/devfile.yaml Outdated Show resolved Hide resolved
@elsony
Copy link
Contributor

elsony commented Apr 11, 2023

@thepetk @mike-hoang Can you test out to see if the existing devfile matching from Alizer will continue to match the existing language-specific stacks/samples as the suggested devfile entries?

- Python
- Pip
- ubi8
projectType: default
Copy link
Member

Choose a reason for hiding this comment

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

what about universal, wouldn't it be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think default is better since it is neutral from the semantics perspective

Copy link
Member

Choose a reason for hiding this comment

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

In every other Devfile this is used to indicate language or framework, default looks weird to me.
Maybe my bigger problem with using default is that it could indicate that users use this Devfile by default, which I have many problems with.

This devfile with UDI can still be helpful. But it should not be presented as a default Devfile. I think that primarily we need to guide users towards using language or frameworks-specific Devfiles. This universal devfile should be considered only if no other Devfile stack can be used or if the project uses multiple languages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This devfile with UDI can still be helpful. But it should not be presented as a default Devfile.

this is about projectType which has nothing to do with devfile being the default per se, right?
universal is a weird name for projectType since it is very ambiguous

In every other Devfile this is used to indicate language or framework

What if there are multiple languages and frameworks used in a particular devfile?
In general, I believe the projectType field should be optional, I do not understand why it is enforced at the moment on the registry end.

Copy link
Member

Choose a reason for hiding this comment

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

What if there are multiple languages and frameworks used in a particular devfile?
In general, I believe the projectType field should be optional, I do not understand why it is enforced at the moment on the registry end.

I agree with this. It is really weird that the registry requires something that is marked as optional in the Devfile spec. (cc: @elsony, @johnmcollier)

this is about projectType which has nothing to do with devfile being the default per se, right?
universal is a weird name for projectType since it is very ambiguous

I was thinking about using universal as it is also in the image name.

The best would be to completely omit that, but we can do that :-(

We could keep default for now, fix the registry to not require it, and then remove it. But I know how this is going to play out; the change will never come and we will get stuck with default forever :-(

Copy link
Contributor

@elsony elsony Apr 13, 2023

Choose a reason for hiding this comment

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

+1 on changing it to universal. I think default is a bit deceiving as well since it implies being the best matching of all.

We require the project type that as part of the registry since we want consistency in the entries in the registry. There are other fields that are optional in the spec but required for the onboarding to the community registry for the same reasons, e.g. icons, description, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to universal even though I do not like the approach that registry enforces certain metadata that is odo specific

@kadel
Copy link
Member

kadel commented Apr 12, 2023

BTW: Devfile without a single command doesn't make much sense from odo perspective.

This is why odo currently fails with devfile like this.

▶ odo dev
 ✗  no command of kind "run" found in the devfile

This will be addressed on odo's side in


This stack will have to be skipped in odo v3 tests by adding --skip="stack: udi" \to

--skip="stack: java-websphereliberty" \


odo v3 tests on this PR should be failing.
It looks like the tests are still not running correctly, probably due to devfile/api#1097 (cc @mike-hoang)

@kadel
Copy link
Member

kadel commented Apr 13, 2023

Another thing to note here is that in order to fully support UDI in odo we will require builds for amd64 (devfile/api#1102)

@elsony
Copy link
Contributor

elsony commented Apr 13, 2023

Can you test out to see if the existing devfile matching from Alizer will continue to match the existing language-specific stacks/samples as the suggested devfile entries?

It has been confirmed that Alizer will still do devfile matching properly with this change. A test case redhat-developer/alizer#175 has been added to Alizer.

@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Apr 14, 2023

@kadel @elsony folks, is there any other action required before merging this PR?

@elsony
Copy link
Contributor

elsony commented Apr 14, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Looks good to me label Apr 14, 2023
@elsony elsony merged commit 6105d4c into devfile:main Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants