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

Fix java-quarkus devfile #341

Merged
merged 4 commits into from
Apr 9, 2024
Merged

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Mar 21, 2024

What does this PR do?:

  • Update the schemaVersion to 2.2.0
  • Add https endpoint

Which issue(s) this PR fixes:

eclipse-che/che#22869

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 Mar 21, 2024

Hi @vinokurig. 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.

@michael-valdron
Copy link
Member

/ok-to-test

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

Looks like you need to rebase the recent nodejs-svelte changes in, this is why odov3 check is failing:

 Output from proc 1:
  --- FAIL: TestOdo (0.02s)
      odo_test.go:72: 
          Expected
              <*fs.PathError | 0xc0008f7e30>: 
              stat /home/runner/work/registry/registry/stacks/nodejs-svelte/1.2.1/devfile.yaml: no such file or directory
              {
                  Op: "stat",
                  Path: "/home/runner/work/registry/registry/stacks/nodejs-svelte/1.2.1/devfile.yaml",
                  Err: <syscall.Errno>0x2,
              }
          to be nil
  FAIL

Output from proc 2:
  --- FAIL: TestOdo (0.02s)
      odo_test.go:72: 
          Expected
              <*fs.PathError | 0xc000653650>: 
              stat /home/runner/work/registry/registry/stacks/nodejs-svelte/1.2.1/devfile.yaml: no such file or directory
              {
                  Op: "stat",
                  Path: "/home/runner/work/registry/registry/stacks/nodejs-svelte/1.2.1/devfile.yaml",
                  Err: <syscall.Errno>0x2,
              }
          to be nil
  FAIL

@maxandersen
Copy link
Collaborator

What/where does the Https serving coming from?

How do I test it?

@vinokurig
Copy link
Contributor Author

vinokurig commented Mar 25, 2024

@michael-valdron
I merged the upstream main branch into the PR branch and changed the source project from zip to git url, please take a look.

Opened a pull request to the sample repository: devfile-samples/devfile-sample-code-with-quarkus#29

@vinokurig
Copy link
Contributor Author

@maxandersen

What/where does the Https serving coming from?

How do I test it?

This is needed to run the application on eclipse CHE. There is a similar PR for .net: #312

@vinokurig
Copy link
Contributor Author

/ok-to-test

@vinokurig
Copy link
Contributor Author

/retest

@michael-valdron
Copy link
Member

michael-valdron commented Mar 25, 2024

changed the source project from zip to git url

@vinokurig I misunderstood what you were originally asking, but now I think I do.

You want to change not just the sample but want to include the sample as the starter project instead of what is generated from https://code.quarkus.io so that the source project can be changed to work with the HTTPS endpoint.

In the case, I'm not sure if this can be done. Let me discuss with the team and followup later today.

Apologies for the misunderstanding.

@maxandersen Let me know if you have any ideas to workaround this, otherwise I'll update you on what devfile services team proposes for this.

@michael-valdron
Copy link
Member

changed the source project from zip to git url

@vinokurig I misunderstood what you were originally asking, but now I think I do.

You want to change not just the sample but want to include the sample as the starter project instead of what is generated from https://code.quarkus.io so that the source project can be changed to work with the HTTPS endpoint.

In the case, I'm not sure if this can be done. Let me discuss with the team and followup later today.

Apologies for the misunderstanding.

@maxandersen Let me know if you have any ideas to workaround this, otherwise I'll update you on what devfile services team proposes for this.

@vinokurig Had a discussion with the team and we don't feel you should use the sample as a starter project since we do not maintain it as often as the current starter projects are maintained by the quarkus development team. As @maxandersen is the owner of this stack, I suggest you continue this conversation with them to work out a solution that works best.

@vinokurig
Copy link
Contributor Author

@michael-valdron

Had a discussion with the team and we don't feel you should use the sample as a starter project since we do not maintain it as often as the current starter projects are maintained by the quarkus development team. As @maxandersen is the owner of this stack, I suggest you continue this conversation with them to work out a solution that works best.

I reverted the project source item to original.

@maxandersen
Copy link
Collaborator

Is there a problem with the generated Project from code.quarkus ?

@vinokurig
Copy link
Contributor Author

No problems, I just want to add some configuration files for vscode editor.

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

If there isn't anything additional needed from the starter projects, the changes looks good to me.

Just need a sign off from @maxandersen.

@maxandersen
Copy link
Collaborator

Ok. I'm all good with the current changes but I don't see the vscode files you want to add?

@michael-valdron
Copy link
Member

Ok. I'm all good with the current changes but I don't see the vscode files you want to add?

I think these were to be added to the starter project used in the stack.

@vinokurig you could still include them as resource files instead, these would be stored under the registry with the devfile.

@vinokurig
Copy link
Contributor Author

@maxandersen @michael-valdron

@vinokurig you could still include them as resource files instead, these would be stored under the registry with the devfile.

Do you have an example of such files? Also I have a question about the starterProjects section of the devfile: why there are two zip items with identical content but different source url?

@maxandersen
Copy link
Collaborator

One is quarkus from community the other red hat product.

@michael-valdron
Copy link
Member

Do you have an example of such files?

Our go stack has supporting resources files in the latest version: https://github.com/devfile/registry/tree/72a45de9a3a01834ecbfa4e920926a041622ade6/stacks/go/2.3.0

Any files along with sub-directories under the stack version directory other than the devfile itself will be treated as resource files and extracted with the devfile.

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

Going to test odo locally as we are dealing with issues on GitHub workflows using odo with some of the stacks due to reduced resources.

@vinokurig My apologies, one more small thing I suggest to change while I test, since the schema version is being bumped I think the new stack version should be a minor bump so 1.4.0 instead of 1.3.1.

Signed-off-by: ivinokur <ivinokur@redhat.com>
Signed-off-by: ivinokur <ivinokur@redhat.com>
@vinokurig
Copy link
Contributor Author

@michael-valdron

My apologies, one more small thing I suggest to change while I test, since the schema version is being bumped I think the new stack version should be a minor bump so 1.4.0 instead of 1.3.1.

done.

@vinokurig
Copy link
Contributor Author

@michael-valdron @maxandersen

Any files along with sub-directories under the stack version directory other than the devfile itself will be treated as resource files and extracted with the devfile.

It is not supported in the Eclipse Che yet. Is there a way to contribute directly to the zip archive of the starter project?

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

Going to test odo locally as we are dealing with issues on GitHub workflows using odo with some of the stacks due to reduced resources.

I ran the tests with odov3 and found that it builds and deploys but during deployment it checks the new endpoint added and there is nothing being served on port 3000, and this is why the odov3 check is failing.

You'll need to have a service that redirects to port 8080 what quarkus is serving through the HTTPS protocol, I'm not sure if this is something that could be done in the devfile itself or needs additional changes to the starter projects from code.quarkus.io and code.quarkus.redhat.com.

odov2 failing check is unrelated as it skips stack versions that use 2.2.x schema versions.

@michael-valdron
Copy link
Member

It is not supported in the Eclipse Che yet. Is there a way to contribute directly to the zip archive of the starter project?

FYI @maxandersen

@michael-valdron
Copy link
Member

Related to part of devfile/api#1318

@michael-valdron
Copy link
Member

michael-valdron commented Mar 28, 2024

You'll need to have a service that redirects to port 8080 what quarkus is serving through the HTTPS protocol, I'm not sure if this is something that could be done in the devfile itself or needs additional changes to the starter projects from code.quarkus.io and code.quarkus.redhat.com.

@vinokurig Setting up something like a reverse proxy component in the devfile might resolve this.

@vinokurig
Copy link
Contributor Author

vinokurig commented Mar 29, 2024

@michael-valdron What if I remove the endpoint with port 3000 but set the existed 8080 endpoint as https?

Signed-off-by: ivinokur <ivinokur@redhat.com>
@vinokurig
Copy link
Contributor Author

/retest

1 similar comment
@vinokurig
Copy link
Contributor Author

/retest

@vinokurig
Copy link
Contributor Author

@michael-valdron @maxandersen Could you please help to figure out why the odo2 test is failing?

@michael-valdron
Copy link
Member

michael-valdron commented Apr 3, 2024

@michael-valdron @maxandersen Could you please help to figure out why the odo2 test is failing?

@vinokurig We can ignore odov2 for this PR since these changes only introduce a stack version that uses schema version 2.2.x which is ignored by this check.

We'll need to investigate why odov2 is failing on the present stack version to continue supporting it.

@vinokurig
Copy link
Contributor Author

@michael-valdron

@vinokurig We can ignore odov2 for this PR since these changes only introduce a stack version that uses schema version 2.2.x which is ignored by this check.

Are we good to go with current changes?

Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

/lgtm

@vinokurig Looks good, need sign off from @maxandersen as well.

@openshift-ci openshift-ci bot added lgtm Looks good to me approved labels Apr 5, 2024
Copy link
Collaborator

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

dont see an issue but also dont have way to run it atm. so no objection trusting you've tested it works in the various layers.

Copy link

openshift-ci bot commented Apr 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maxandersen, michael-valdron, vinokurig

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

@vinokurig
Copy link
Contributor Author

@michael-valdron @maxandersen I don't have enough permissions, could you please merge?

@michael-valdron michael-valdron merged commit ae3dbe4 into devfile:main Apr 9, 2024
9 of 10 checks passed
thepetk pushed a commit to thepetk/registry that referenced this pull request Apr 22, 2024
* Add a new version of java quarkus stack

Signed-off-by: ivinokur <ivinokur@redhat.com>

* Change the new version to 1.4.

Signed-off-by: ivinokur <ivinokur@redhat.com>

* fixup! Add a new version of java quarkus stack

Signed-off-by: ivinokur <ivinokur@redhat.com>

---------

Signed-off-by: ivinokur <ivinokur@redhat.com>
Signed-off-by: thepetk <thepetk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants