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

Update for MP6 #494

Merged
merged 23 commits into from
Oct 17, 2024
Merged

Update for MP6 #494

merged 23 commits into from
Oct 17, 2024

Conversation

majidmostafavi
Copy link
Contributor

Add Telemetry
update for MP6

@majidmostafavi
Copy link
Contributor Author

@Emily-Jiang
I find the error @health is deprecated. so I should replace it by @Liveness ,@readiness.

@Emily-Jiang
Copy link
Member

@Emily-Jiang I find the error @health is deprecated. so I should replace it by @Liveness ,@readiness.

Depending on which version of MP health you are talking about. It was only deprecated later. It was fine in the earlier version of MP Health. I suggest you to fix the test error first not to change other things.

@Emily-Jiang
Copy link
Member

@luiseufrasio did you figure out what was the test failures and propose a update on the PR to unblock this?

@luiseufrasio
Copy link
Contributor

clipsefdn/eca

@Emily-Jiang the tests seems to be NOT running: https://github.com/eclipse/microprofile-starter/pull/494/checks
I says "Workflow runs completed with no jobs"

@Emily-Jiang
Copy link
Member

The conflict needs to be solved before the tests can be executed. @majidmostafavi can you please resolve the conflicts and then the CI jobs can be launched?

@@ -0,0 +1,4 @@
{"configs":{"
MP33":{"supportedServers":["
]},"MP32":{"supportedServers":[
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this file. Do you need to just mention MP60?

Copy link
Member

Choose a reason for hiding this comment

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

@rdebusscher we are stuck on this failure. Can you please shed some lights on this? Your help is very much appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Emily-Jiang What is the issue exactly? From the failed test run, it just seems a problem with some assertions on text values.
Not much time to go in detail for the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@luiseufrasio luiseufrasio Apr 17, 2024

Choose a reason for hiding this comment

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

I tried the URL locally running on liberty:
image

So after I called:
http://127.0.0.1:9080/mp-starter/api/project?supportedServer=LIBERTY&selectedSpecs=CONFIG&selectedSpecs=FAULT_TOLERANCE&selectedSpecs=HEALTH_CHECKS&selectedSpecs=METRICS&selectedSpecs=OPEN_TRACING&selectedSpecs=OPEN_API&artifactId=liberty&buildTool=MAVEN

I received back:

{
	"error": "One or more selectedSpecs is not available for the given mpVersion",
	"code": "ERROR003"
}

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the version combination.
What about the error for APITest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Error in the API test is due to the addition of the Telemetry option. Please update the test to the new expected values.

Copy link
Member

Choose a reason for hiding this comment

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

Which file needs to be updated? @rdebusscher

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the log of the test

https://github.com/eclipse/microprofile-starter/pull/494/checks#step:7:17864

This is the failure

supportMatrix(org.eclipse.microprofile.starter.APITest)  Time elapsed: 0.319 sec  <<< FAILURE!
java.lang.AssertionError: Response of /7/supportMatrix 

And from the test method APITest#supportMatrix

    public void supportMatrix() throws FileNotFoundException {
        test(v7Matrix, "/7/supportMatrix");

we see that the contents of this file os compared to the response

v7Matrix = new File(getClass().getClassLoader().getResource("json_examples/v7/supportMatrix.json.segments").getFile());        

@rdebusscher
Copy link
Contributor

Why is there a v7 of the Api created when it is identical to v6?

@Emily-Jiang
Copy link
Member

Why is there a v7 of the Api created when it is identical to v6?

v7 has Telemetry added though.

@rdebusscher
Copy link
Contributor

The version of the API has nothing to do with the config op the MP versions and the options. It is about the available endpoints, their parameters and responses.

So /api/mpVersion/MP60, /api/7/mpVersion/MP60, and , /api/6/mpVersion/MP60 all return

{"specs":["CONFIG","FAULT_TOLERANCE","JWT_AUTH","METRICS","HEALTH_CHECKS","OPEN_API","REST_CLIENT","TELEMETRY","GRAPHQL"],"supportedServers":["LIBERTY"]}

Even the API versions 2 and higher, all return this result which includes Telemetry.

So there is no need at all to have a new API version.

before MP50 and MP60 use Health
before MP50 and MP60 use Health
@Emily-Jiang
Copy link
Member

The version of the API has nothing to do with the config op the MP versions and the options. It is about the available endpoints, their parameters and responses.

So /api/mpVersion/MP60, /api/7/mpVersion/MP60, and , /api/6/mpVersion/MP60 all return

{"specs":["CONFIG","FAULT_TOLERANCE","JWT_AUTH","METRICS","HEALTH_CHECKS","OPEN_API","REST_CLIENT","TELEMETRY","GRAPHQL"],"supportedServers":["LIBERTY"]}

Even the API versions 2 and higher, all return this result which includes Telemetry.

So there is no need at all to have a new API version.

I thought the earlier version of MP (1.x-5.x) should NOT return Telemetry @rdebusscher

@rdebusscher
Copy link
Contributor

Earlier versions of MP will not return Telemetry. As I already said, MP Version and API version of the Starter app are 2 independent things.

@Emily-Jiang
Copy link
Member

Earlier versions of MP will not return Telemetry. As I already said, MP Version and API version of the Starter app are 2 independent things.

Should the earlier API version such as 2.x return Telemetry at all?

@rdebusscher
Copy link
Contributor

Earlier versions of MP will not return Telemetry. As I already said, MP Version and API version of the Starter app are 2 independent things.

Should the earlier API version such as 2.x return Telemetry at all?

In my opinion, yes. Those that make an integration with the API should not use latest version since that can have breaking changes at any time. Only a certain API version is stable.
And having all options in all API versions doesn't force external parties to update their integration with the MP Starter API.

@Emily-Jiang
Copy link
Member

Earlier versions of MP will not return Telemetry. As I already said, MP Version and API version of the Starter app are 2 independent things.

Should the earlier API version such as 2.x return Telemetry at all?

In my opinion, yes. Those that make an integration with the API should not use latest version since that can have breaking changes at any time. Only a certain API version is stable. And having all options in all API versions doesn't force external parties to update their integration with the MP Starter API.

Thanks @rdebusscher ! @majidmostafavi you don't need to create a new version of the API.


import javax.ws.rs.Path;

@Path("/7")
Copy link
Member

Choose a reason for hiding this comment

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

Based on my understanding from @rdebusscher , there is no need to do this change.

@breakponchito
Copy link
Contributor

breakponchito commented Oct 8, 2024

@Emily-Jiang @cesarhernandezgt I found the way to fix the issues from the API MicroProfile Starter but I can't add commits on this PR, it seems someone needs to give me some permission:

remote: Permission to majidmostafavi/microprofile-starter.git denied to breakponchito. fatal: unable to access 'https://github.com/majidmostafavi/microprofile-starter.git/': The requested URL returned error: 403 00:12:20.257: [microprofile-starter] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false push --progress --porcelain majid refs/heads/majidMain:main remote: Permission to majidmostafavi/microprofile-starter.git denied to breakponchito. fatal: unable to access 'https://github.com/majidmostafavi/microprofile-starter.git/': The requested URL returned error: 403
probably I should need to recreate the PR

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Oct 8, 2024

@Emily-Jiang @cesarhernandezgt I found the way to fix the issues from the API MicroProfile Starter but I can't add commits on this PR, it seems someone needs to give me some permission:

remote: Permission to majidmostafavi/microprofile-starter.git denied to breakponchito. fatal: unable to access 'https://github.com/majidmostafavi/microprofile-starter.git/': The requested URL returned error: 403 00:12:20.257: [microprofile-starter] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false push --progress --porcelain majid refs/heads/majidMain:main remote: Permission to majidmostafavi/microprofile-starter.git denied to breakponchito. fatal: unable to access 'https://github.com/majidmostafavi/microprofile-starter.git/': The requested URL returned error: 403 probably I should need to recreate the PR

@breakponchito it was the branch from @majidmostafavi . I think you can checkout his branch and then do a new PR.

git checkout -b majidmostafavi-main main
git pull https://github.com/majidmostafavi/microprofile-starter.git main

@breakponchito
Copy link
Contributor

@Emily-Jiang @cesarhernandezgt I found the way to fix the issues from the API MicroProfile Starter but I can't add commits on this PR, it seems someone needs to give me some permission:
remote: Permission to majidmostafavi/microprofile-starter.git denied to breakponchito. fatal: unable to access 'https://github.com/majidmostafavi/microprofile-starter.git/': The requested URL returned error: 403 00:12:20.257: [microprofile-starter] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false push --progress --porcelain majid refs/heads/majidMain:main remote: Permission to majidmostafavi/microprofile-starter.git denied to breakponchito. fatal: unable to access 'https://github.com/majidmostafavi/microprofile-starter.git/': The requested URL returned error: 403 probably I should need to recreate the PR

@breakponchito it was the branch from @majidmostafavi . I think you can checkout his branch and then do a new PR.

git checkout -b majidmostafavi-main main
git pull https://github.com/majidmostafavi/microprofile-starter.git main

yes I already create a new PR here: #499
it seems that I need an approval from a maintainer

@Emily-Jiang Could you help me with this?
image

@Emily-Jiang Emily-Jiang merged commit a633bbc into microprofile:main Oct 17, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants