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

chore: use 'https' protocol for .net 6.0 devfile. Update schema to 2.2.0 #298

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

ibuziuk
Copy link
Collaborator

@ibuziuk ibuziuk commented Feb 19, 2024

What does this PR do?:

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:

try it out on Developer Sandbox - https://workspaces.openshift.com/#https://raw.githubusercontent.com/devfile/registry/1f99545009bad5ab1f3231c66bcd6bf29d583573/stacks/dotnet60/devfile.yaml

@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Feb 19, 2024

@kadel @yangcao77 folks, could you clarify why 2.2.2 is not supported ?

rror: failed to generate index struct: /build/stacks/dotnet60 devfile is not valid: failed to populateAndParseDevfile: unable to find schema for version "2.2.2". The parser supports devfile schema for version 2.0.0, 2.1.0, 2.2.0, v1alpha2

thepetk
thepetk previously approved these changes Feb 19, 2024
Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

@kadel as you are the owner of the stack, could you please also review the PR?

@openshift-ci openshift-ci bot added lgtm Looks good to me approved labels Feb 19, 2024
@thepetk thepetk removed the lgtm Looks good to me label Feb 19, 2024
@thepetk thepetk self-requested a review February 19, 2024 16:19
@thepetk thepetk dismissed their stale review February 19, 2024 16:19

Accidentally approved

@openshift-ci openshift-ci bot removed the approved label Feb 19, 2024
@thepetk
Copy link
Contributor

thepetk commented Feb 19, 2024

@kadel @yangcao77 folks, could you clarify why 2.2.2 is not supported ?

rror: failed to generate index struct: /build/stacks/dotnet60 devfile is not valid: failed to populateAndParseDevfile: unable to find schema for version "2.2.2". The parser supports devfile schema for version 2.0.0, 2.1.0, 2.2.0, v1alpha2

My bad I've tested with schemaVersion 2.2.0 instead of 2.2.2. I've removed my approval 🥲
I think atm the supported versions of the devfile library are listed here so the v2.2.2 is not yet supported from the parser. @ibuziuk would it make sense to change the schemaVersion from 2.2.2 to 2.2.0?

@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Feb 20, 2024

@thepetk sure I can changed the version, but for me it looks like the problem should be fixed on the parser end.

@thepetk
Copy link
Contributor

thepetk commented Feb 21, 2024

/retest

@thepetk
Copy link
Contributor

thepetk commented Feb 21, 2024

@thepetk sure I can changed the version, but for me it looks like the problem should be fixed on the parser end.

@ibuziuk FYI I've created devfile/api#1454 in order to update the parser and add support for devfile schema - versions 2.2.1 & 2.2.2

@ibuziuk ibuziuk force-pushed the ibuziuk-patch-1 branch 3 times, most recently from 1f99545 to 58a86b4 Compare February 22, 2024 10:41
@ibuziuk ibuziuk changed the title chore: use 'https' protocol for .net 6.0 devfile. Update schema to the latest 2.2.2 chore: use 'https' protocol for .net 6.0 devfile. Update schema to 2.2.0 Feb 22, 2024
@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Feb 22, 2024

@kadel @thepetk @elsony please, review

@thepetk
Copy link
Contributor

thepetk commented Feb 22, 2024

/retest

1 similar comment
@thepetk
Copy link
Contributor

thepetk commented Feb 22, 2024

/retest

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

@ibuziuk I've tested locally the new devfile and it looks good to me.

One minor detail I would suggest is to create a new default version (e.g. 1.0.3) of the dotnet60 instead of directly updating the 1.0.2. The updated stacks/dotnet60 should have the following contents:

- stack.yaml
- 1.0.2/
    - devfile.yaml
    ...
- 1.0.3/
    - devfile.yaml
    ...

@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Feb 27, 2024

One minor detail I would suggest is to create a new default version (e.g. 1.0.3) of the dotnet60 instead of directly updating the 1.0.2. The updated stacks/dotnet60 should have the following contents

@thepetk could you clarify why would you like to create intricate versioning in the repo and not just bump the version to 1.0.3 ? e.g. I do not see 1.0.1 / 1.0.0 versions atm

@thepetk
Copy link
Contributor

thepetk commented Feb 27, 2024

One minor detail I would suggest is to create a new default version (e.g. 1.0.3) of the dotnet60 instead of directly updating the 1.0.2. The updated stacks/dotnet60 should have the following contents

@thepetk could you clarify why would you like to create intricate versioning in the repo and not just bump the version to 1.0.3 ? e.g. I do not see 1.0.1 / 1.0.0 versions atm

@ibuziuk IMO this would be a better approach and is followed for other stacks (e.g go, nodejs-* stacks) too. This way we can take advantage of the multiple versions support of the registry viewer and every user can trace easier the different versions of a stack. As I've mentioned it is a minor suggestion so I'm fine if we just bump up to 1.0.3 too :)

@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Feb 27, 2024

@thepetk sounds good, thank you
I bumped the version of the devfile, but not going to add the previous 1.0.2 because I simply struggle to understand the value of it (1.0.2 does not work properly at least with Eclipse Che and I do not want it to be exposed)

…e latest 2.2.0

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk
Copy link
Collaborator Author

ibuziuk commented Feb 29, 2024

@thepetk can the PR be merged now?

@thepetk
Copy link
Contributor

thepetk commented Feb 29, 2024

@thepetk can the PR be merged now?

@ibuziuk we need a review from @kadel who is listed as the stack owner.

Copy link
Contributor

@thepetk thepetk 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 openshift-ci bot added lgtm Looks good to me approved labels Feb 29, 2024
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Mar 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ibuziuk, kadel, thepetk

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

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.

3 participants