-
Notifications
You must be signed in to change notification settings - Fork 466
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
[WFCORE-6560] Log installation provisioning information at boot #5724
Conversation
I also added an SPI operation to expose the latest manifest name in wildfly-extras/wildfly-installation-manager-api#18 and wildfly-extras/prospero#481 |
history = installationManager.history(); | ||
CONFIG_LOGGER.provisioningHistory(history); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe what we could need from the history is the latest revision instead of printing all the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yersan I added a commit dropping that history logging. And without that verbose logging don't see any need for using the org.jboss.as.config logger.
@yersan Will this stuff work in embedded servers / hosts? I have a vague recollection you disabled it for embedded just to narrow the scope of your work on this. I'm not asking for any change; I'm just refreshing my memory. :) Yesterday I thought about installing the service even on embedded and just not registering the Resource and ResourceDefinition. But then I decided I wanted to keep the scope narrow for this logging as well. Logging this for embedded is a corner case we can come back to if there is demand. |
@spyrkob I didn't approve those as they are draft PRs, but they LGTM. |
Hi @bstansberry , good point there. The tool is not available there, narrowing the scope was one reason. In general:
Although it could be possible to circumvent the above points and allow the use of the CLI integration, for simplicity and the first cut, we disabled the usability of the installation manager in the embedded server/host. Thinking on the embedded, as soon as we are running in a modular environment and Prospero is installed with the server, we should be able to use the factory to create an instance to get any kind of version. We can continue avoiding registering the management resources when using the embedded process type to continue being compliant with the above points but use Prospero to compute any version info. |
Thanks @yersan. I think for this part I'll describe having this logging in an embedded process as a 'Nice to Have', and move to 'Future Work' if we don't get to it (which is likely.) It's easy to tweak this to support embedded, but testing it is more work, and if we don't test it having it adds risk. |
a3d67b8
to
16cdae3
Compare
16cdae3
to
0e6e149
Compare
FYI, it turns out testing this will have to happen in full WF, as the org.wildfly.prospero:wildfly-galleon-pack depends on Jackson modules only provided by the wildfly-ee FP. So I'll submit a draft component-upgrade-needed wildfly/wildfly PR for those tests, link it here and link some custom integration test runs. I'll move it out of draft when that is done. |
ok, in case it helps and if you see it useful, our testing strategy for Installation Manager in WildFly Core has been using a mocked integration. It means tests in WildFly Core for the installation manager are testing until the API method invocation. Prospero also has an extended test suite that also tests the internal invocation of their own implementation. You could just use a mocked version and return something to test the functionality. Not sure if that would cope with your requirements, but that would avoid changes in WildFly full. |
e0c1360
to
90a1a1d
Compare
@spyrkob To test this I'll need a release of Prospero with ProsperoInstallationManager.getInstalledVersions() implemented. |
Core -> Full Integration Build 13275 outcome was UNKNOWN using a merge of 90a1a1d |
@bstansberry prospero 1.2.1.Final with the updated InstallationManager has just been released |
90a1a1d
to
0e6e149
Compare
0e6e149
to
9bc3ae5
Compare
Thanks, @yersan -- that's very helpful. I've done exactly that in this PR, so there's no need for a test in full WF. I missed your comment and wrote a test in full WF anyway, but I don't want it merged anymore, as core is where such tests belong. So, assuming the current CI runs are ok, this is ready to go. |
cbdfbde
to
bc06465
Compare
Core -> Full Integration Build 13297 outcome was FAILURE using a merge of bc06465 |
Core -> Full Integration Build 13068 outcome was FAILURE using a merge of bc06465 |
Core -> WildFly Preview Integration Build 13137 outcome was FAILURE using a merge of bc06465 |
Core -> Full Integration Build 13069 outcome was FAILURE using a merge of bc06465 |
Core -> WildFly Preview Integration Build 13138 outcome was FAILURE using a merge of bc06465 |
Core -> Full Integration Build 13298 outcome was FAILURE using a merge of bc06465 |
@@ -81,6 +86,18 @@ public interface InstMgrLogger extends BasicLogger { | |||
@Message(id = 20, value = "No custom patches installed found for the specified manifest maven coordinates: '%s'") | |||
OperationFailedException noCustomPatchFound(String manifestGA); | |||
|
|||
@LogMessage(level = Logger.Level.INFO) | |||
@Message(id = 21, value = "Installation provisioning is using the following channels: '%s'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but I wonder if this should read something like "Installation provisioned using the following channel states/(versions?):". The ManifestVersions provide information about the latest used versions from each channel rather then channel information like manifest location, repositories etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @spyrkob -- good point. I went with "Installation was provisioned using the following channel versions:".
testsuite/manualmode/pom.xml
Outdated
<!-- Validates content of traditional unzipped installation. (Works, but is irrelevant to bootable jar testing.) --> | ||
<exclude>org.jboss.as.test.manualmode.provisioning.ProvisioningConsistencyTestCase</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unrelated to these changes, isn't it? I am not against it, just trying to link the points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yersan Yes, it's unrelated. I have a bunch of branches in this general 'test provisioning' topic area and I got tired of switching back and forth. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you for confirming @bstansberry , I also wanted to be sure it was not unintentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just waiting for the minor pending questions and getting the feature progressing more before approving, but overall LGTM
bc06465
to
8ea536b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
1776cf7
to
251db36
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
251db36
to
061b5fa
Compare
@yersan @spyrkob This change to WF isn't really a feature and doesn't need to be handled as such. So I changed the JIRA to Enhancement and removed the 'Feature' etc labels from this PR. So if you all are ok with approving it we can get it merged and out of the WIP queue. Downstream it's part of a larger requirement set, but that's not relevant to how we need to handle it in WF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @bstansberry @spyrkob |
A test I meant to add to this PR but neglected to push can be found at #5967 |
https://issues.redhat.com/browse/WFCORE-6560
Draft for now what we log is expected to change. This just dumps what's currently available with no particular attempt at formatting.
@yersan @spyrkob FYI