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

[WFCORE-6893] Add information to product-info #6070

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spyrkob
Copy link
Contributor

@spyrkob spyrkob commented Jul 11, 2024

@spyrkob
Copy link
Contributor Author

spyrkob commented Jul 11, 2024

@yersan could you take a look at this? I created a hidden/private operation handler for the installation manager history that is called from both the :product-info and installer history operation handlers.

@wildfly-ci
Copy link

Core -> WildFly Preview Integration Build 13733 outcome was FAILURE using a merge of dacd47c
Summary: Exit code 1 (Step: Build core (Maven)) (new) Build time: 00:01:25

@wildfly-ci
Copy link

Core -> Full Integration Build 13661 outcome was FAILURE using a merge of dacd47c
Summary: Exit code 1 (Step: Build core (Maven)) (new) Build time: 00:01:28

@wildfly-ci
Copy link

Core -> Full Integration Build 13948 outcome was FAILURE using a merge of dacd47c
Summary: Exit code 1 (Step: Build core (Maven)) (new) Build time: 00:01:47

Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

@spyrkob I'm going to pick up this work and try to find an approach that does not require splitting the history handler into two versions


@Override
public void execute(OperationContext context, ModelNode operation) throws OperationFailedException {
try {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for yuour info, even if this operation has the .setRuntimeOnly() flag, this execute method is executed at Model stage.

@yersan
Copy link
Collaborator

yersan commented Jul 16, 2024

Finally, it was decided to use addFirst trick on the installation manager history handler to coordinate the execution, context about the discussion can be found in Zulip

@spyrkob
Copy link
Contributor Author

spyrkob commented Jul 16, 2024

@yersan should Il keep this as draft until the installation-manager 1.0.3.Final is out and update it in this PR?

@wildfly-ci
Copy link

Core -> Full Integration Build 13668 outcome was FAILURE using a merge of bd81049
Summary: Exit code 1 (Step: Build core (Maven)) Build time: 00:01:38

@wildfly-ci
Copy link

Core -> Full Integration Build 13955 outcome was FAILURE using a merge of bd81049
Summary: Exit code 1 (Step: Build core (Maven)) Build time: 00:01:56

@wildfly-ci
Copy link

Core -> WildFly Preview Integration Build 13740 outcome was FAILURE using a merge of bd81049
Summary: Exit code 1 (Step: Build core (Maven)) Build time: 00:02:03

@yersan
Copy link
Collaborator

yersan commented Jul 16, 2024

@spyrkob Ideally yes, we can release installation-manager once we test this PR together with Prospero. Then we bump it on wildfly core and this would be rebased so we can proceed with the merge. We would need to test the operation when Prospero (or the installation manager test version) is enabled

@yersan
Copy link
Collaborator

yersan commented Jul 16, 2024

@spyrkob Talking about testing strategy, this is what we have so far:

Share your feedback if you see a better testing strategy, real provisioning with Prospero for testing seems more tricky since this is only WildFly Core and we do not want to be tight to one Prospero version, here we are only testing until the Prospero API invocation.

@spyrkob
Copy link
Contributor Author

spyrkob commented Jul 17, 2024

@yersan I added tests to check that the correct information is added to the product info and installer history. I based the integration tests for standalone mode on the existing domain tests. I'm not sure if there's a better way to avoid the code duplication between the two

@wildfly-ci
Copy link

Core -> Full Integration Build 13961 outcome was FAILURE using a merge of 8b38f17
Summary: Exit code 1 (Step: Build core (Maven)) Build time: 00:01:29

@wildfly-ci
Copy link

Core -> WildFly Preview Integration Build 13746 outcome was FAILURE using a merge of 8b38f17
Summary: Exit code 1 (Step: Build core (Maven)) Build time: 00:01:47

@wildfly-ci
Copy link

Core -> Full Integration Build 13674 outcome was FAILURE using a merge of 8b38f17
Summary: Exit code 1 (Step: Build core (Maven)) Build time: 00:01:49

@yersan
Copy link
Collaborator

yersan commented Jul 18, 2024

I'm not sure if there's a better way to avoid code duplication between the two

@spyrkob I think it could have been enough by just adding the :product-info operation test to the standalone version. The code path for domain mode and standalone is the same, the only difference is that the standalone address is /core-service=installer instead of /host=name/core-service=installer, but server handlers are the same, CLI handlers just add the host name.

It is good to get both covered though, but if we do it, I guess we should remove the duplication (which can be done later). We can use this directory https://github.com/wildfly/wildfly-core/tree/main/testsuite/shared/src/main/java/org/wildfly/test/installationmanager to place a base class that accepts a list of host names and inherit both test claasses from that base class, the domain one will add the hosts to the test methods.

It is ok to me if this is done after this fix, I leave it up to you. If not we can create a Jira to do it later.

Copy link
Collaborator

@yersan yersan left a comment

Choose a reason for hiding this comment

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

Looks good to me, there are two minor details, the one about test duplicity, up to you can be done later, and the minor about the date format of the timestamp.

I'm going to merge and release the one of the installation manager API, to get it later available here

List<ModelNode> result = Operations.readResult(installerInfo).asList();
for (ModelNode installerAtt : result) {
if (installerAtt.has("timestamp")) {
return installerAtt.get("timestamp").asString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep the format given by the legacy tool:

return DateFormat.getInstance().format(new Date());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK that probably should happen in InstMgrHistoryHandler, in the Reporter it's too late - we already have a text value

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 19, 2024
@wildfly-ci

This comment was marked as outdated.

@spyrkob
Copy link
Contributor Author

spyrkob commented Jul 22, 2024

/retest

@wildfly-ci

This comment was marked as outdated.

@spyrkob spyrkob force-pushed the WFCORE-6893 branch 2 times, most recently from e921710 to ff49019 Compare July 26, 2024 14:42
@spyrkob spyrkob marked this pull request as ready for review August 9, 2024 11:34
Copy link

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

@github-actions github-actions bot added the Stale label Sep 29, 2024
@yersan
Copy link
Collaborator

yersan commented Nov 13, 2024

@spyrkob Reviewing your PR I found that there is an issue with the installation manager integration tests running on Windows. The failures here are not related to your PR. I opened https://issues.redhat.com/browse/WFCORE-7047 to fix them

@yersan
Copy link
Collaborator

yersan commented Nov 14, 2024

@spyrkob Failures in this PR should be resolved with #6240

There was left one File.lists without a try/catch, so the stream was never closed and Windows (legitimately) complained about it.

Let me know if instead of merging #6240 and rebasing yours on top of that would be easier for this PR to just incorporate #6240 changes directly here.

@spyrkob
Copy link
Contributor Author

spyrkob commented Nov 15, 2024

@yersan I'm happy to do either one, whatever is easier for you/makes more sense from process perspective

@yersan
Copy link
Collaborator

yersan commented Nov 18, 2024

@spyrkob Since there were no strong preferences, I've just merged #6240, you can rebase and incorporate the two minimal changes done in such a PR on the current one and the failures in Windows should disappear

@wildfly-ci

This comment was marked as off-topic.

@spyrkob
Copy link
Contributor Author

spyrkob commented Nov 20, 2024

@yersan I think the error in the Windows test is not related to this change, but I don't have the rights to rerun the test

@yersan
Copy link
Collaborator

yersan commented Nov 20, 2024

@spyrkob yes, unrelated. In any case, anyone with a user different from anonymous in ci.wildfly.org can re-trigger a job.
I'll do a final review for this one today

@yersan yersan added Feature This PR adds a new feature to WildFly missing-reqs This PR is missing external requirements before it can be merged labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes Feature This PR adds a new feature to WildFly missing-reqs This PR is missing external requirements before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants