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

[WFLY-18578] Allow channel-based overriding of the org.jboss.as.produ… #17248

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

bstansberry
Copy link
Contributor

@bstansberry bstansberry commented Oct 4, 2023

…ct data

Use artifacts to store the JBoss-Product-Release-Name and JBoss-Product-Release-Version MANIFEST.MF entries to make it possible to update that info via a channel

https://issues.redhat.com/browse/WFLY-18578

Incorporates https://issues.redhat.com/browse/WFLY-18791 as it's the foundation for the test.

@jmesnil @wolfc @spyrkob @jfdenise @darranl @bmaxwell whether we decide to make use of this capability in the near term or not, I think this is the correct way to do this. The old approach was a mix of pom magic, galleon magic and JBoss Modules magic. Too much magic.

@bstansberry bstansberry force-pushed the WFLY-18758 branch 2 times, most recently from b2580fd to d336474 Compare October 4, 2023 21:19
@github-actions github-actions bot added the deps-changed Dependencies have been checked, and there are changes highlighted in a comment label Oct 4, 2023
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Dependency Tree Analyzer Output:

New Dependencies:

  • org.wildfly:wildfly-ee-feature-pack-product-conf:jar:31.0.0.Beta1-SNAPSHOT:compile
  • org.wildfly:wildfly-feature-pack-product-conf:jar:31.0.0.Beta1-SNAPSHOT:compile

CC @wildfly/prod

@spyrkob
Copy link
Contributor

spyrkob commented Oct 5, 2023

Is the expectation that if we wanted to separate the reported version from build version, we'd build the product-conf jar separately and inject it into the channel?

@jfdenise
Copy link
Contributor

jfdenise commented Oct 5, 2023

@bstansberry , I tried locally, I opened PR against your branch for small adjustments: bstansberry#65

@bstansberry
Copy link
Contributor Author

Is the expectation that if we wanted to separate the reported version from build version, we'd build the product-conf jar separately and inject it into the channel?

@spyrkob Yes. Getting all that going would be the harder part; this part is simpler.

@bstansberry
Copy link
Contributor Author

@bstansberry , I tried locally, I opened PR against your branch for small adjustments: bstansberry#65

Thanks, @jfdenise -- I merged that.

@bstansberry
Copy link
Contributor Author

@spyrkob Note that there are other ways we could approach things; this is just one and it involves the least change to the main server code. We can discuss options elsewhere. I decided to push this forward since looking at how we make this data available currently is overly magical and it's valid to update it.

@bstansberry
Copy link
Contributor Author

bstansberry commented Oct 31, 2023

I've found the cause of the Windows failures.

https://issues.redhat.com/browse/WFGP-259

I'll put a hold on this pending integration of a WFGP release with that fixed.

@bstansberry bstansberry added the hold PR should not be merged for some reason. label Oct 31, 2023
@bstansberry
Copy link
Contributor Author

bstansberry commented Nov 3, 2023

I'll retest once #17368 is merged.

@bstansberry bstansberry added rebase-this PR has a merge conflict. and removed hold PR should not be merged for some reason. 31.x labels Nov 7, 2023
@bstansberry bstansberry removed the rebase-this PR has a merge conflict. label Nov 21, 2023
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this change was off-topic. It's a duplicate entry in the bom that maven was complaining about, so I removed it.

@jfdenise
Copy link
Contributor

jfdenise commented Dec 1, 2023

@bstansberry , I am wandering if we really need the channel artifact. What the WildFly Maven plugin is using is the channel manifest (maven coordinates or URL). The maven repos in use to build the channel are the ones coming from the maven context.

@bstansberry
Copy link
Contributor Author

@jfdenise Yeah, I agree. I added these (in the similar PR in core) before I realized they were irrelevant.

WDYT though about just having the 'channel' module and have it produce two artifacts? Except the -channel.yaml artifact would be commented out. I don't much like having two modules, and different artifactIds.

The context here is part of this is trying to lay the groundwork for a possible future day when we'd actually deploy a channel as part of a release. Might as well get things set up correctly. BUT, that's a side goal; doesn't block anything. The point here is just to set up test infrastructure.

@bstansberry
Copy link
Contributor Author

@jfdenise I combined the channel.yaml and manifest.yaml stuff into a single maven module per FP. I disabled attaching the channel.yaml as an artifact for now.

@bstansberry bstansberry force-pushed the WFLY-18758 branch 2 times, most recently from 1de1113 to bc44603 Compare December 1, 2023 23:31
@jfdenise
Copy link
Contributor

jfdenise commented Dec 4, 2023

@bstansberry , the Maven plugin that builds the feature-pack deploys the channel (https://github.com/wildfly/galleon-plugins/blob/main/maven-plugin/src/main/java/org/wildfly/galleon/maven/AbstractFeaturePackBuildMojo.java#L398). We should make that optional and disable it in WildFly. Perhaps that it could be enough and the channel module not required?

@bstansberry
Copy link
Contributor Author

@jfdenise I'm not sure whether that needs to be configurable or not. Well, it does, but it doesn't seem super urgent. However, we should never promote (e.g. blogs, docs etc) such a manifest independent of a conscious decision by the relevant project that that's what it wants to do. We don't have that for WF or WF Core.

I'd prefer to leave the module in WF. I don't like the artifactId of our feature packs and if we started promoting a WF channel I'd need convincing to use that artifactId.

@jfdenise
Copy link
Contributor

jfdenise commented Dec 5, 2023

@bstansberry , thank-you. I created https://issues.redhat.com/browse/WFGP-262 to track it.

@bstansberry bstansberry force-pushed the WFLY-18758 branch 2 times, most recently from a5036d5 to c299d5f Compare December 6, 2023 01:33
- id: "jboss-public"
url: "https://repository.jboss.org/nexus/content/groups/public/"
manifest:
maven: "org.wildfly:${project.artifactId}:${project.version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

the maven coordinates should be broken up like:

manifest:
  maven:
    groupId: "org.wildfly"
    artifactId: "${project.artifactId}"
    version: "${project.version}"

Also does the channel need to define the manifest version? I think it would make more sense to have the major version included into the artifactId of the manifest so the updates can be consumed if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spyrkob I fixed the formatting; thanks.

Re the manifest version, I'd prefer to address that later. Right now this is just a piece of test infrastructure, and I really need to move on from this testing effort to focus elsewhere. But for sure we need to get this right when we start to publish channels.

pom.xml Outdated
<artifactId>wildfly-channel</artifactId>
<version>${full.maven.version}</version>
<type>yaml</type>
<classifier>channel</classifier>
Copy link
Contributor

Choose a reason for hiding this comment

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

since the channels are not attached to the reactor, I think this dependency could be removed? Or does it make sense to keep it in case we need to attache the channels in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that can be removed. When I started all this I added some random off-topic bits in case we decided to push and deploy channels as part of WF 31. But now it's right at the deadline and we need more discussion before doing that, so these can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

<artifactId>wildfly-ee-galleon-pack</artifactId>
<version>${ee.maven.version}</version>
</feature-pack>
<feature-pack>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to include prospero feature pack? IIUC the test only verifies the interaction between wildfly channel and the product-conf-override-manifest.yaml. The integration tests in prospero already verify installing Wildfly + prospero feature pack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right; @spyrkob -- that can be removed. That's left over from when I also used this execution for the testing I later moved into wildfly/wildfly-core#5724.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

bstansberry and others added 6 commits December 12, 2023 16:55
…ure packs

Disable attaching the channel.yaml files until there is a concrete need for them.
…ct data

Use artifacts to store the JBoss-Product-Release-Name and JBoss-Product-Release-Version MANIFEST.MF entries to make it possible to update that info via a channel
@bstansberry
Copy link
Contributor Author

Thanks @spyrkob -- I believe I've addressed the issues you raised.

Copy link
Contributor

@spyrkob spyrkob left a comment

Choose a reason for hiding this comment

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

LGTM!

@bstansberry bstansberry added the ready-for-merge Only for use by those with merge permissions! label Dec 13, 2023
@bstansberry bstansberry merged commit 4c82ed6 into wildfly:main Dec 13, 2023
15 checks passed
@bstansberry bstansberry deleted the WFLY-18758 branch December 13, 2023 14:47
@bstansberry
Copy link
Contributor Author

Thanks @spyrkob and @jfdenise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-changed Dependencies have been checked, and there are changes highlighted in a comment ready-for-merge Only for use by those with merge permissions!
Projects
None yet
3 participants