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

Publish maven artifacts to GH Packages #305

Merged
merged 2 commits into from
Jun 5, 2024
Merged

Publish maven artifacts to GH Packages #305

merged 2 commits into from
Jun 5, 2024

Conversation

sgraband
Copy link
Contributor

@sgraband sgraband commented Jun 3, 2024

Provide reusable maven ci workflow.
Publish maven-conf and org.eclipse.theia.cloud.common.
Publish org.eclipse.theia.cloud.operator and org.eclipse.theia.cloud.service.
Always resolve org.eclipse.theia.cloud.common locally.
Update actions/setup-java@v3 to actions/setup-java@v4.

Successful publish runs (hardcoded to run on PR):
maven-conf
common
operator
service

Some observations/questions:

  1. We could probably remove the service tests CI as this is now indirectly als called on the maven build. WDYT?
  2. The naming on the checks (which are starting to become quite a few) could be improved imho. Maybe in a follow up?

@sgraband sgraband force-pushed the mavenPublish branch 15 times, most recently from ace8466 to 07569da Compare June 5, 2024 06:42
Provide reusable maven ci workflow.
Publish `maven-conf` and `org.eclipse.theia.cloud.common`.
Publish `org.eclipse.theia.cloud.operator` and `org.eclipse.theia.cloud.service`.
Always resolve `maven-conf` and `org.eclipse.theia.cloud.common` locally.
Update `actions/setup-java@v3` to `actions/setup-java@v4`.
@sgraband sgraband requested a review from lucas-koehler June 5, 2024 06:49
@sgraband sgraband marked this pull request as ready for review June 5, 2024 06:49
@sgraband sgraband changed the title WIP: Publish maven artifacts to GH Packages Publish maven artifacts to GH Packages Jun 5, 2024
Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

The workflows already look pretty good to me ✨ . I have two minor suggestions and two questions inline.

We could probably remove the service tests CI as this is now indirectly also called on the maven build. WDYT?

I agree that we can remove it.

The naming on the checks (which are starting to become quite a few) could be improved imho. Maybe in a follow up?

That's true. Yes, we can do a follow up.

.github/workflows/reusable-maven.yml Outdated Show resolved Hide resolved
.github/workflows/reusable-maven.yml Show resolved Hide resolved
.github/workflows/ci-maven-common.yml Show resolved Hide resolved
.github/workflows/reusable-maven.yml Outdated Show resolved Hide resolved
Add flags to maven build to clean up logs and execute tests.
Improve input description.
Remove the service tests, as they are indirectly run via the maven build.
@sgraband
Copy link
Contributor Author

sgraband commented Jun 5, 2024

Thanks for the feedback. I applied your feedback and removed the service tests. I also created a reminder to discuss the renaming of the ci jobs.

@sgraband sgraband requested a review from lucas-koehler June 5, 2024 12:48
Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. LGTM now ✨

jobs:
call-reusable-maven-workflow:
permissions:
packages: write
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit surprised that this works despite not adding the contents: read permission. But as the workflow works this is apparently not strictly necessary.

Although, the documentation says:

If you specify the access for any of these scopes, all of those that are not specified are set to none.

It's a bit strange but I think we can leave it like this for now.

@sgraband sgraband merged commit 2d1d439 into main Jun 5, 2024
25 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.

2 participants