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

Updated the devfile metadata to be more consistent #134

Merged
merged 13 commits into from
Oct 18, 2022

Conversation

schultzp2020
Copy link
Contributor

@schultzp2020 schultzp2020 commented Oct 4, 2022

What does this PR do?:

  1. Updates the devfile metadata to be more consistent i.e. capitalize Java in the language property.
  2. Adds editorconfig and gitattributes
  3. Adds github action concurrency check

Which issue(s) this PR fixes:

N/A

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:

Signed-off-by: Paul Schultz <pschultz@pobox.com>
Signed-off-by: Paul Schultz <pschultz@pobox.com>
@schultzp2020
Copy link
Contributor Author

schultzp2020 commented Oct 4, 2022

Please my changes to your stacks. Thanks!
@kadel
@ajm01
@maxandersen
@ehsavoie
@BethGriggs

@kadel
Copy link
Member

kadel commented Oct 5, 2022

/lgtm

Signed-off-by: Paul Schultz <pschultz@pobox.com>
@openshift-ci openshift-ci bot removed the lgtm Looks good to me label Oct 6, 2022
Copy link
Collaborator

@ehsavoie ehsavoie left a comment

Choose a reason for hiding this comment

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

For java-wildfly-bootable and java-wildfly

@openshift-ci openshift-ci bot added the lgtm Looks good to me label Oct 6, 2022
@schultzp2020
Copy link
Contributor Author

@maxandersen and @BethGriggs please review my changes before its planned merge date of 2022/10/13.

tags:
- Java
- Quarkus
projectType: Quarkus
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the type not still lower case quarkus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Language and projectType should follow the same case as the commercial display name.

Copy link
Contributor

@scottkurz scottkurz left a comment

Choose a reason for hiding this comment

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

Do we need to bump version?

schemaVersion: 2.1.0
metadata:
name: java-openliberty
version: 0.8.1
displayName: 'Open Liberty Maven'
version: 0.8.2
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the downside of leaving the version as-is for the four Liberty (Open/WebSphere , Maven/Gradle) devfiles?

We started down a path of using our own versioning strategy integrating our outer loop artifacts. Following that to its conclusion we'd want to make other changes.

But if this change is just syntax cleanup, well, I think we could say this is equivalent to our previous version, e.g. 0.8.1 in this case, and merge in the change.

(And we could open an issue in our devfile source repo to make the same changes there: https://github.com/OpenLiberty/devfile-stack/issues ). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we recommend version updates (semver) when the stack is updated so that users know the underlying stack has been changed. Having said that, it is just a recommendation so it is up to the stack owner to have the final decision on the version number.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather just leave the version alone in this PR then.

I opened: OpenLiberty/devfile-stack#212 to follow-up and make these changes. We can still view that repo as the source of record of our devfile.

But I think it's OK to go ahead and merge the rest of the changes. We might not get to it for a few weeks so don't want to keep you waiting.

There's an opportunity for minor user confusion here, I know, but unless there's some specific reason (like some version of some devfile tooling having a bug in the older syntax) then I don't think it will end up mattering all that much.

@openshift-ci
Copy link

openshift-ci bot commented Oct 12, 2022

@scottkurz: changing LGTM is restricted to collaborators

In response to this:

Do we need to bump version?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Paul Schultz <pschultz@pobox.com>
@openshift-ci openshift-ci bot removed the lgtm Looks good to me label Oct 12, 2022
Signed-off-by: Paul Schultz <pschultz@pobox.com>
Signed-off-by: Paul Schultz <pschultz@pobox.com>
Signed-off-by: Paul Schultz <pschultz@pobox.com>
Signed-off-by: Paul Schultz <pschultz@pobox.com>
@kadel
Copy link
Member

kadel commented Oct 18, 2022

odo v3 test is failing due to timeout when downloading icr.io/appcafe/open-liberty-devfile-stack image. The problem is that the image is big and icr.io is probably slow, so it sometimes takes more than 2mins to pull the image. Minikube has a default 2 min timeout for pulling images (more info in devfile/api#960) I'm working on a solution as part of devfile/api#960.

For now, just add

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

line to tests/check_odov3.sh. I'll remove it once I figure out how to reconfigure minikube to use bigger timeout.

While at it, add

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

as well. It also has an image in icr.io sometimes, it fails as well.

Signed-off-by: Paul Schultz <pschultz@pobox.com>
Signed-off-by: Paul Schultz <pschultz@pobox.com>
Signed-off-by: Paul Schultz <pschultz@pobox.com>
@openshift-ci openshift-ci bot added the lgtm Looks good to me label Oct 18, 2022
@openshift-ci
Copy link

openshift-ci bot commented Oct 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehsavoie, johnmcollier, schultzp2020

The full list of commands accepted by this bot can be found here.

The pull request process is described 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 johnmcollier merged commit 8a9b173 into devfile:main Oct 18, 2022
@schultzp2020 schultzp2020 deleted the devfile-maintainance branch October 18, 2022 20:20
@kadel
Copy link
Member

kadel commented Oct 19, 2022

The tests with odo v3 were failing, but it still got merged :-(

Now the dotnet devfiles are not working with odo.
This is a bug in odo (redhat-developer/odo#6234), but still, this is precisely the reason we have tests here, so we know when something gets broken, and we can fix it.

If we ignore failing tests then I don't know why we even have them.

@johnmcollier
Copy link
Member

@kadel Hey, sorry about that, you’re absolutely right, and merging this is in was premature. We had discussed this PR on a team call yesterday,, and were under the impression it would be safe to merge in, but that was clearly a mistake.

It also appears that the odo v3 tests are not a required check to merge, and that should definitely change.

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

Successfully merging this pull request may close these issues.

8 participants