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 devfile.yaml #252

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Updated devfile.yaml #252

wants to merge 4 commits into from

Conversation

ashwaq06
Copy link

@ashwaq06 ashwaq06 commented Nov 26, 2023

What does this PR do?:

Summarize the changes. Are any stacks or samples added or updated?
This PR addresses the inconsistency in port numbers between the devfile.yaml file and the corresponding Kubernetes deployment in the Python 3.0.0 stack. The targetPort for the http-python endpoint in the devfile.yaml is updated to align with the Kubernetes deployment, resolving the issue.

Which issue(s) this PR fixes:

Link to github issue(s)
devfile/api#998

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:

Copy link

openshift-ci bot commented Nov 26, 2023

Hi @ashwaq06. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

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.

@ashwaq06 thanks for taking action for this issue! I've replied to the comment made by @maysunfaisal :)

stacks/python/3.0.0/devfile.yaml Outdated Show resolved Hide resolved
@thepetk
Copy link
Contributor

thepetk commented Dec 1, 2023

@ashwaq06 thanks for taking action for this issue! I've replied to the comment made by @maysunfaisal :)

If it is ok for you, I'll assign devfile/api#998 to you as you have already started working on this!

I've also checked the failing tests for this PR. I've noticed two things:

  1. We should update the library version used inside the validate_devfile_schemas check. Right now the check is failing because of multiple endpoints, from different components, having the same value (8080). This has been fixed in the latest release of library/v2 and now we accept multiple endpoints having the same port.

  2. The other issue comes from odo check. Very recently odo added support for the latest version of the devfile library (here), but a new release is pending (makes sense as the update was made today). As a result, we should also update the odo version used for check_odov3 check in order to pass this failure.

As those updates IMO are not covered by the good-first-issue label I've created a separate issue (api#1367) to address the update of the checks in order to support the suggested changes.

cc @maysunfaisal

@ashwaq06
Copy link
Author

ashwaq06 commented Dec 4, 2023

@ashwaq06 thanks for taking action for this issue! I've replied to the comment made by @maysunfaisal :)
If it is ok for you, I'll assign devfile/api#998 to you as you have already started working on this!

I've also checked the failing tests for this PR. I've noticed two things:

  1. We should update the library version used inside the validate_devfile_schemas check. Right now the check is failing because of multiple endpoints, from different components, having the same value (8080). This has been fixed in the latest release of library/v2 and now we accept multiple endpoints having the same port.
  2. The other issue comes from odo check. Very recently odo added support for the latest version of the devfile library (here), but a new release is pending (makes sense as the update was made today). As a result, we should also update the odo version used for check_odov3 check in order to pass this failure.

As those updates IMO are not covered by the good-first-issue label I've created a separate issue (api#1367) to address the update of the checks in order to support the suggested changes.

cc @maysunfaisal

@thepetk I am excited to make a contribution and I'm willing to work on both issues. It would be great if you could assign them to me. Can you please provide a simplified overview of all the changes that are required for both tasks so that I can ensure that I'm on the right track? As someone new to contributions, any guidance or assistance you can offer to help me understand the process better would be much appreciated!

cc @maysunfaisal

@thepetk
Copy link
Contributor

thepetk commented Dec 4, 2023

Hey @ashwaq06 :)

It would be great if you could assign them to me.

Ops my mistake, I think due to permissions I cannot do that. But there is no problem as you were able to create a PR!

Can you please provide a simplified overview of all the changes that are required for both tasks so that I can ensure that I'm on the right track?

  1. For the current PR, I think the correct approach is mentioned here Updated devfile.yaml #252 (comment). That said, the current PR should be updated and should include only the port 8080 in:
  1. For issue Update devfile library and odo version for registry tests  api#1367 as it has a level of complexity I'd suggest to wait until its refinement, the process which will clarify all the acceptance criteria of the issue, so we now for sure the required steps.

Copy link

openshift-ci bot commented Dec 4, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ashwaq06
Once this PR has been reviewed and has the lgtm label, please ask for approval from thepetk. 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

@ashwaq06
Copy link
Author

ashwaq06 commented Dec 4, 2023

@thepetk Should I have to change all the ports in https://github.com/devfile/registry/blob/main/stacks/python/3.0.0/kubernetes/deploy.yaml
Including

ports:
- name: http-8081
port: 8081
protocol: TCP
targetPort: 8081

and

containers:
- name: my-python
image: python-image:latest
ports:
- name: http
containerPort: 8081
protocol: TCP

@thepetk
Copy link
Contributor

thepetk commented Dec 4, 2023

@thepetk Should I have to change all the ports in https://github.com/devfile/registry/blob/main/stacks/python/3.0.0/kubernetes/deploy.yaml Including

ports:
- name: http-8081
port: 8081
protocol: TCP
targetPort: 8081

and

containers:
- name: my-python
image: python-image:latest
ports:
- name: http
containerPort: 8081
protocol: TCP

In general it looks good to me. Although we will have to wait until the blocker issue is resolved :)

@ashwaq06
Copy link
Author

ashwaq06 commented Dec 4, 2023

In general it looks good to me. Although we will have to wait until the blocker issue is resolved :)

Ok. Feel free to let me know if there are any changes

@thepetk
Copy link
Contributor

thepetk commented May 23, 2024

/retest

Copy link
Contributor

@Jdubrick Jdubrick left a comment

Choose a reason for hiding this comment

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

You will need to sign your commits in order to pass the DCO check. More info on how to do that if you're unsure here

cc @ashwaq06

@ashwaq06
Copy link
Author

You will need to sign your commits in order to pass the DCO check. More info on how to do that if you're unsure here

cc @ashwaq06

Yeah I'll update the commits

@thepetk
Copy link
Contributor

thepetk commented May 23, 2024

The checks now should be passing after the completion of devfile/api#1494

@ashwaq06
Copy link
Author

The checks now should be passing after the completion of devfile/api#1494

Can we merge this?

@thepetk
Copy link
Contributor

thepetk commented May 23, 2024

The checks now should be passing after the completion of devfile/api#1494

Can we merge this?

@ashwaq06 in order to merge it it's required to have an approval from the owner (devfile-services-team) & the che-team (see here for more details) and all the checks to be successful. As @Jdubrick mentions above as a first step:

You will need to sign your commits in order to pass the DCO check. More info on how to do that if you're unsure here

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

Successfully merging this pull request may close these issues.

4 participants