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

Unify backend source code #2609

Merged
merged 19 commits into from
Sep 24, 2024
Merged

Unify backend source code #2609

merged 19 commits into from
Sep 24, 2024

Conversation

heliocastro
Copy link
Contributor

Reference: #2602

@heliocastro heliocastro self-assigned this Sep 11, 2024
@KoukiHama
Copy link
Member

Thank you for big works. I will review it.

@heliocastro
Copy link
Contributor Author

@KoukiHama @GMishx Can we go ahead on this, we already have one conflict and will be only get worse ahead

@GMishx GMishx added needs general test This is general testing, meaning that there is no org specific issue to check for and removed ready ready to merge labels Sep 20, 2024
@GMishx
Copy link
Member

GMishx commented Sep 20, 2024

@heliocastro The changes looks good to me and I just tested the branch and the /api/components endpoint is now throwing 500 error. Other endpoints are working flawlessly, I also tested them with the frontend project and Swagger.
Just for http://localhost:8080/resource/api/components, I get following error but nothing in logs.

{
  "timestamp": "2024-09-20T05:54:24.790291950Z",
  "status": 500,
  "error": "Internal Server Error",
  "message": "HTTP Response code: 404"
}

Other than this break, I approve the PR.

Also, please understand we need to move carefully (definitely not mean slowly) for such major changes. More so, when the build time is down from 120 seconds to 45 seconds 😉

I do understand your pain with constant conflict resolution, but please also understand this PR will cause merge conflict for 50 other PRs at the same time.

@heliocastro
Copy link
Contributor Author

@heliocastro The changes looks good to me and I just tested the branch and the /api/components endpoint is now throwing 500 error. Other endpoints are working flawlessly, I also tested them with the frontend project and Swagger. Just for http://localhost:8080/resource/api/components, I get following error but nothing in logs.

{
  "timestamp": "2024-09-20T05:54:24.790291950Z",
  "status": 500,
  "error": "Internal Server Error",
  "message": "HTTP Response code: 404"
}

Other than this break, I approve the PR.

Also, please understand we need to move carefully (definitely not mean slowly) for such major changes. More so, when the build time is down from 120 seconds to 45 seconds 😉

I do understand your pain with constant conflict resolution, but please also understand this PR will cause merge conflict for 50 other PRs at the same time.

Yes, but will always have PRs, so if we go in this mode, we will never do the changes. I just rebased again with CycloneDX, and this will go forever, so we need merge it, and fix the PR, but not wit the PR's because wealways come some more, and more

@GMishx
Copy link
Member

GMishx commented Sep 20, 2024

Yes, but will always have PRs, so if we go in this mode, we will never do the changes. I just rebased again with CycloneDX, and this will go forever, so we need merge it, and fix the PR, but not wit the PR's because wealways come some more, and more

I agree to the point, that's why I approved it. However, we cannot merge a PR which breaks existing functionality, that too as basic as /components endpoint, not something obscured.

Once the components endpoint is fixed, I am good with merging this PR.

I guess @KoukiHama wanted to review it as well.

@KoukiHama
Copy link
Member

I'm currently in the process of reviewing. I've gone through about half of it and will finish reviewing everything by the weekend.

@KoukiHama
Copy link
Member

I agree to the point, that's why I approved it. However, we cannot merge a PR which breaks existing functionality, that too as basic as /components endpoint, not something obscured.

Once the components endpoint is fixed, I am good with merging this PR.

I agree with this approach as well. While it's important to make progress, we shouldn't merge the PR until the basic functionality is fixed. Once the error is resolved, we can proceed with the merge.

Copy link
Member

@KoukiHama KoukiHama left a comment

Choose a reason for hiding this comment

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

Alomost all ok to me

docker-compose.yml Outdated Show resolved Hide resolved
@smrutis1 smrutis1 removed their request for review September 24, 2024 09:41
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
- Update parent backend pom to proper naming jar files sw360 prefixed

Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
…rocess

Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
…n server

Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
Signed-off-by: Helio Chissini de Castro <heliocastro@gmail.com>
@heliocastro
Copy link
Contributor Author

@heliocastro The changes looks good to me and I just tested the branch and the /api/components endpoint is now throwing 500 error. Other endpoints are working flawlessly, I also tested them with the frontend project and Swagger. Just for http://localhost:8080/resource/api/components, I get following error but nothing in logs.

{
  "timestamp": "2024-09-20T05:54:24.790291950Z",
  "status": 500,
  "error": "Internal Server Error",
  "message": "HTTP Response code: 404"
}

Other than this break, I approve the PR.

Also, please understand we need to move carefully (definitely not mean slowly) for such major changes. More so, when the build time is down from 120 seconds to 45 seconds 😉

I do understand your pain with constant conflict resolution, but please also understand this PR will cause merge conflict for 50 other PRs at the same time.

@GMishx Fixed. Components portlet was not properly exported

@heliocastro heliocastro merged commit af056ef into main Sep 24, 2024
2 checks passed
@heliocastro heliocastro deleted the feature/unify_backend branch September 24, 2024 18:29
@GMishx
Copy link
Member

GMishx commented Sep 25, 2024

Hi @heliocastro,

Thank you for your efforts in addressing the endpoint issue. Unfortunately, we haven't had the opportunity to review or test the changes yet. It appears that the PR was self-merged, which is generally not recommended for community projects.

Could we please discuss the topic of self-merging during today's community call to ensure we're all on the same page?

/cc @KoukiHama @ag4ums @arunazhakesan

@KoukiHama
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs general test This is general testing, meaning that there is no org specific issue to check for
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants