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.

Sign up for GitHub

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

Jump to bottom

Polishing the core module by adding missing Javadoc and do some refactoring #180

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Polishing the core module by adding missing Javadoc and do some refactoring #180

merged 3 commits into from
Sep 4, 2024

Conversation

fabiobrz
Copy link
Member

Description

Some changes for the core module, i.e. adding missing Javadoc, refactoring the ProcessKeystoregenerator class, and moving the auto provisioning tooling to the provisioners module

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is self-descriptive and/or documented
  • I have implemented unit tests to cover my changes
  • I tested my code in OpenShift

@fabiobrz fabiobrz added enhancement New feature or request documentation Improvements or additions to documentation labels Jul 19, 2024
@fabiobrz
Copy link
Member Author

/test

@fabiobrz
Copy link
Member Author

/and-test-again

@fabiobrz
Copy link
Member Author

test-me

@fabiobrz
Copy link
Member Author

test-again

@fabiobrz
Copy link
Member Author

ok-to-test

@fabiobrz
Copy link
Member Author

/test

@fabiobrz
Copy link
Member Author

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests,
which were triggered by @fabiobrz comment, see #180 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#72> FAILED:

Intersmash integration tests job <eap-8.x-intersmash-integration-tests-community#28> reported failures.
Please verify your changes locally or reach out to maintainers in order to investigate the issues.

Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap#41> reported failures.
Please verify your changes locally or reach out to maintainers in order to investigate the issues.

Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#45> reported failures.
Please verify your changes locally or reach out to maintainers in order to investigate the issues.

@fabiobrz
Copy link
Member Author

/test

@fabiobrz
Copy link
Member Author

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests,
which were triggered by @fabiobrz comment, see #180 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#74> FAILED:

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-community#29> reported failures.
    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap#42> reported failures.
    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#46> reported failures.
    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

@fabiobrz
Copy link
Member Author

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests,
which were triggered by @fabiobrz comment, see #180 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#76> FAILED:

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-community#31> reported failures.
    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap#43> reported failures.
    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap-xp#47> reported failures.
    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

@fabiobrz fabiobrz assigned fabiobrz and unassigned fabiobrz Sep 1, 2024
@fabiobrz
Copy link
Member Author

fabiobrz commented Sep 1, 2024

/test

@fabiobrz fabiobrz marked this pull request as ready for review September 1, 2024 15:32
@fabiobrz fabiobrz closed this Sep 1, 2024
@fabiobrz fabiobrz reopened this Sep 1, 2024
@fabiobrz fabiobrz changed the title Polish.core Polishing the core module by adding missing Javadoc and do some refactoring Sep 1, 2024
@fabiobrz
Copy link
Member Author

fabiobrz commented Sep 2, 2024

/test

@fabiobrz
Copy link
Member Author

fabiobrz commented Sep 3, 2024

/test

@fabiobrz
Copy link
Member Author

fabiobrz commented Sep 3, 2024

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests,
which were triggered by @fabiobrz comment, see #180 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#105> FAILED:

  • Intersmash integration tests job <eap-8.x-intersmash-integration-tests-products-jboss-eap#59> reported failures.

    Please verify your changes locally or reach out to maintainers in order to investigate the issues.

@fabiobrz
Copy link
Member Author

fabiobrz commented Sep 3, 2024 •
edited
Loading

Intersmash PR CI check results

Hi @fabiobrz, this comment is meant to report the outcome of Intersmash integration tests, which were triggered by @fabiobrz comment, see #180 (comment).

CI checks job <continuous-testing-umb-listener-for-intersmash-pr-checks#105> FAILED:

* Intersmash integration tests job <_eap-8.x-intersmash-integration-tests-products-jboss-eap#59_> reported failures.
  Please verify your changes locally or reach out to maintainers in order to investigate the issues.

<eap-8.x-intersmash-integration-tests-products-jboss-eap#59> failed due to a Jenkins error, and was executed again as <eap-8.x-intersmash-integration-tests-products-jboss-eap#60>, which in turn reported 2 failures:

  • org.jboss.intersmash.testsuite.provision.auto.AutoProvisioningTests.testAppIsLive
  • org.jboss.intersmash.testsuite.provision.openshift.ProvisionerCleanupTestCase.testProvisioningWorkflowCleanup(OpenShiftProvisioner)[1] (related to InfinispanOperatorProvisionerTest)

Both are intermittent failures, and actually reported as fixed in the subsequent run, i.e. <eap-8.x-intersmash-integration-tests-products-jboss-eap#62>.

Based on the above findings, the changes in the PR are deemed to pass the integration tests and can now be reviewed.

@fabiobrz fabiobrz assigned marekkopecky and unassigned fabiobrz Sep 3, 2024
marekkopecky
marekkopecky reviewed Sep 4, 2024
View reviewed changes
core/src/main/java/org/jboss/intersmash/util/CommandLineBasedKeystoreGenerator.java
}

private static String getSplitCommandNameBasedOnOeratingSystem() {
return System.getProperty("os.name").toLowerCase().startsWith("mac") ? "gcsplit" : "csplit";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is mac supported by other parts of intersmash as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, there are no other places where we take it into account.
That must be a leftover of early implementation of this utility class.
I might file an issue for removal, if you'd be okay with that, or remove it now, although this would mean another "costly" integration-tests round.
WDYT @marekkopecky ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, let's merge it now, the issue for the future would be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @marekkopecky - here's the issue tracker: #198

@marekkopecky marekkopecky merged commit b0ec5e6 into Intersmash:main Sep 4, 2024
1 check passed
@fabiobrz fabiobrz mentioned this pull request Sep 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marekkopecky marekkopecky marekkopecky left review comments

Assignees

@marekkopecky marekkopecky

Labels
documentation Improvements or additions to documentation enhancement New feature or request it-passed
Projects
Intersmash library
Status: Done
Milestone
Intersmash 0.0.3 - Open Data Hub/Open...
Development

Successfully merging this pull request may close these issues.
2 participants
@fabiobrz @marekkopecky