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

FISH-6607: Introduce core-parent #6246

Merged
merged 34 commits into from
May 23, 2023
Merged

FISH-6607: Introduce core-parent #6246

merged 34 commits into from
May 23, 2023

Conversation

pdudits
Copy link
Contributor

@pdudits pdudits commented Mar 24, 2023

Description

Oooh, it's a big one. Build separation of core modules from the other ones, while not moving them away from their current locations in source tree.

  • The modules that had core in groupId now have parent core-parent, and version 6.0.0-SNAPSHOT
  • all these modules are removed from their previous parents and aggregates and are aggregated with core-aggregator (which is not their parent now, but one level higher)
  • any dependencies core modules had are now managed via core-bom. (and payara-bom reexports core-bom)
  • any properties required for those and parent setup moved to core-aggregator
  • therefore core-aggregator is parent of payara-aggregator so that we don't have duplicated properties

All of that was plugged back to old aggregator, so that no immediate change to build process is required.

It wasn't the aim of remove duplication of all of the plugin configs and such, rather to keep core-parent minimal, as it will grow another responsibilities. Therefore non-core modules did not get core-parent as (transitive) parent.

I think this is best reviewed by commits:

  • 406fefb introduces core-aggreator, moving properties into it
  • d570565 adds core-parent
  • next commits add modules that turned out to be dependencies of core
  • except for metro-xmlsec-repackaged which shouldn't have been in core
  • 969dcfc then replaces all places that referred to core modules with explicit ${project.version}. There were lots of them.
  • Then relevant modules to allow for making admingui plugins were added (console-common and its dependencies)
  • And as a verification I transformed certificate-management to only depend on payara-core in b93b560

Testing

Testing Performed

Builds with empty repo, starts, payara-samples pass.
core/ builds with empty repo on its own.

Testing Environment

Maven 3.9.2, Ubuntu 22.04, Zulu-11

@pdudits pdudits requested review from aubi and Pandrex247 March 24, 2023 16:37
@pdudits
Copy link
Contributor Author

pdudits commented Mar 24, 2023

Jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

Doesn't build with a clean repo using the BuildExtras profile

[ERROR] Failed to execute goal on project certificate-management-cli: Could not resolve dependencies for project fish.payara.extras.certificate-management:certificate-management-cli:glassfish-jar:6.2023.4-SNAPSHOT: Could not find artifact fish.payara.server.internal.cluster:cluster-common:jar:6.2023.4-SNAPSHOT

@pdudits
Copy link
Contributor Author

pdudits commented Mar 27, 2023

Fixed that one. But it opens a question of certificate-management in general. Do you feel it should be eligible to be out-of-tree component? Because it still has few internal dependencies that should move to core in such case - server-mgmt, cluster-cli and some admingui ones.

@Pandrex247
Copy link
Member

Yes certificate management should be an external component.
The only reason it still exists in the codebase is because it was created before we had such a process and we never went back to it.

@Pandrex247
Copy link
Member

Given that we're releasing Enterprise 6.0.0 now - should the core version still be 6.0.0 or should it be 6.1.0 (or higher)?

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

Yeah I think I need a sit down with you and a whiteboard.

pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
api/payara-api/pom.xml Outdated Show resolved Hide resolved
core/core-parent/pom.xml Outdated Show resolved Hide resolved
core/pom.xml Show resolved Hide resolved
@Pandrex247 Pandrex247 self-requested a review March 27, 2023 15:38
@Pandrex247
Copy link
Member

Also is this intentionally not building the l10n modules as a part of core?

@Pandrex247
Copy link
Member

Also also, isn't the api/pom.xml a little pointless now?
It's now a parent pom for one child module (payara-bom).

@pdudits
Copy link
Contributor Author

pdudits commented Apr 6, 2023

it doesn't seem to be possible to generate sources or javadoc specifically for the core modules

I agree, is this something we can put on backlog, or does it needs solving right now?

Also is this intentionally not building the l10n modules as a part of core?

They're not needed for building components, only for distribution, so yes, it was intentional.

Also also, isn't the api/pom.xml a little pointless now?

It is, bom should be treated as distribution artifact, but we're not quite there yet.

@Pandrex247
Copy link
Member

Pandrex247 commented Apr 11, 2023

it doesn't seem to be possible to generate sources or javadoc specifically for the core modules

I agree, is this something we can put on backlog, or does it needs solving right now?

It depends where we're intending to deploy these.
If we're intending to deploy all of these core artefacts to Maven Central, then I believe it's still a requirement that anything that isn't a pom/bom still has to be deployed with javadoc and sources. While these can just be dummy files (e.g. containing a README), we'd still need to somehow script this for all of the artefacts.

If we're deploying to our own maven repository, then it would still be nice to have this set up. Past this singular point in time where currently everything is 6.0.0-SNAPSHOT, once it has been released the actual dependency version we're all going to be working from will be 6.0.0, whereas the version present in the checked out repo will presumably be 6.1.0 - our IDE's will throw a hissy fit.

Also is this intentionally not building the l10n modules as a part of core?

They're not needed for building components, only for distribution, so yes, it was intentional.

So the l10n stuff will have a completely separate release process? So we release core to wherever we publish it with its own version and it's own string substitutions, but then have what actually gets subtituted in released separately and with a completely separate version?

Also, thinking ahead to where the core exists in the future where it's possibly in its own repo, we're going to have the l10n stuff where? In this repo awkwardly divorced from what it's actually the text for?

Also also, isn't the api/pom.xml a little pointless now?

It is, bom should be treated as distribution artifact, but we're not quite there yet.

Are we not? Could you not just remove it in this PR? As of this PR it's a useless pom - you could leave the directory structure the same but simply remove the pointless pom and adjust the payara-bom to be a child of the top-level aggregator

@pdudits
Copy link
Contributor Author

pdudits commented Apr 21, 2023

Jenkins test please

@pdudits pdudits requested a review from Pandrex247 April 21, 2023 13:24
@pdudits
Copy link
Contributor Author

pdudits commented Apr 21, 2023

  • I dropped payara.core.version from dependency management -- mvn release:prepare completed up to the point where it wasn't properly configured and tried to immediately push a tag to github
  • Merged in current master, respecing version upgrades to Hazelcast and EclipseLink
  • Reset versions back to 6.2023.5-SNAPSHOT with mvn versions:set. Current state should not block releasing next version until we decide further details on how the core vs main release will be done, and fine tune the breadth of core modules.

@pdudits
Copy link
Contributor Author

pdudits commented Apr 24, 2023

Jenkins test please

pdudits added 2 commits May 11, 2023 14:10
Version changes applied in parent commit

# Conflicts:
#	pom.xml
@pdudits
Copy link
Contributor Author

pdudits commented May 11, 2023

Jenkins test please

# Conflicts:
#	api/payara-api/pom.xml
#	api/payara-core-bom/pom.xml
#	appserver/admin/admin-core/pom.xml
#	appserver/admingui/common/pom.xml
#	appserver/admingui/dataprovider/pom.xml
#	appserver/admingui/faces-compat/pom.xml
#	appserver/admingui/gf-admingui-connector/pom.xml
#	appserver/admingui/plugin-service/pom.xml
#	appserver/common/amx-javaee/pom.xml
#	appserver/common/annotation-framework/pom.xml
#	appserver/common/container-common/pom.xml
#	appserver/common/glassfish-ee-api/pom.xml
#	appserver/common/glassfish-naming/pom.xml
#	appserver/common/stats77/pom.xml
#	appserver/connectors/connectors-internal-api/pom.xml
#	appserver/deployment/client/pom.xml
#	appserver/deployment/dol/pom.xml
#	appserver/deployment/javaee-core/pom.xml
#	appserver/ejb/ejb-internal-api/pom.xml
#	appserver/extras/certificate-management/pom.xml
#	appserver/orb/orb-connector/pom.xml
#	appserver/orb/orb-enabler/pom.xml
#	appserver/packager/external/libpam4j/pom.xml
#	appserver/resources/resources-connector/pom.xml
#	appserver/security/core-ee/pom.xml
#	appserver/security/jaspic-provider-framework/pom.xml
#	appserver/security/webintegration/pom.xml
#	appserver/transaction/internal-api/pom.xml
#	appserver/web/admin/pom.xml
#	appserver/web/gf-web-connector/pom.xml
#	appserver/web/gui-plugin-common/pom.xml
#	appserver/web/war-util/pom.xml
#	appserver/web/web-core/pom.xml
#	appserver/web/web-embed/api/pom.xml
#	appserver/web/web-glue/pom.xml
#	appserver/web/web-naming/pom.xml
#	appserver/web/web-sse/pom.xml
#	nucleus/admin/cli/pom.xml
#	nucleus/admin/config-api/pom.xml
#	nucleus/admin/launcher/pom.xml
#	nucleus/admin/monitor/pom.xml
#	nucleus/admin/rest/rest-client/pom.xml
#	nucleus/admin/server-mgmt/pom.xml
#	nucleus/admin/util/pom.xml
#	nucleus/cluster/admin/pom.xml
#	nucleus/cluster/cli/pom.xml
#	nucleus/cluster/common/pom.xml
#	nucleus/cluster/ssh/pom.xml
#	nucleus/common/amx-core/pom.xml
#	nucleus/common/common-util/pom.xml
#	nucleus/common/glassfish-api/pom.xml
#	nucleus/common/internal-api/pom.xml
#	nucleus/common/mbeanserver/pom.xml
#	nucleus/common/scattered-archive-api/pom.xml
#	nucleus/common/simple-glassfish-api/pom.xml
#	nucleus/core/api-exporter/pom.xml
#	nucleus/core/bootstrap/pom.xml
#	nucleus/core/kernel/pom.xml
#	nucleus/core/logging/pom.xml
#	nucleus/deployment/common/pom.xml
#	nucleus/flashlight/framework/pom.xml
#	nucleus/grizzly/config/pom.xml
#	nucleus/grizzly/nucleus-grizzly-all/pom.xml
#	nucleus/hk2/config-types/pom.xml
#	nucleus/hk2/hk2-config/pom.xml
#	nucleus/packager/external/j-interop/pom.xml
#	nucleus/packager/external/jmxremote_optional/pom.xml
#	nucleus/packager/external/ldapbp/pom.xml
#	nucleus/packager/external/opentelemetry-repackaged/pom.xml
#	nucleus/packager/external/opentracing-repackaged/pom.xml
#	nucleus/packager/external/tiger-types/pom.xml
#	nucleus/packager/external/trilead-ssh2/pom.xml
#	nucleus/payara-modules/hazelcast-bootstrap/pom.xml
#	nucleus/payara-modules/healthcheck-core/pom.xml
#	nucleus/payara-modules/healthcheck-stuck/pom.xml
#	nucleus/payara-modules/opentracing-adapter/pom.xml
#	nucleus/payara-modules/payara-executor-service/pom.xml
#	nucleus/payara-modules/requesttracing-core/pom.xml
#	nucleus/resources/pom.xml
#	nucleus/security/core/pom.xml
#	nucleus/security/services/pom.xml
#	nucleus/security/ssl-impl/pom.xml
#	nucleus/test-utils/utils/pom.xml
#	pom.xml
@pdudits
Copy link
Contributor Author

pdudits commented May 16, 2023

Jenkins test please

@pdudits
Copy link
Contributor Author

pdudits commented May 22, 2023

Jenkins test please

1 similar comment
@pdudits
Copy link
Contributor Author

pdudits commented May 22, 2023

Jenkins test please

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