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

chore(content): Add Quarkus 3.14 Updates to Docs #1675

Merged
merged 6 commits into from
Sep 12, 2024

Conversation

psavidis
Copy link
Contributor

@psavidis psavidis commented Sep 4, 2024

@psavidis psavidis self-assigned this Sep 4, 2024
@psavidis psavidis changed the title 4163 update quarkus 314 properties docs chore(content): Add Quarkus 3.14 Updates to Docs Sep 4, 2024
@psavidis psavidis marked this pull request as ready for review September 4, 2024 13:49
Copy link
Member

@mboskamp mboskamp left a comment

Choose a reason for hiding this comment

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

🔧 This is very good already. I was wondering if we could provide a little more background information to our users about why we decided to introduce a breaking change.

content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
@psavidis
Copy link
Contributor Author

psavidis commented Sep 9, 2024

Hello @christinaausley,

I'd like to kindly request your input for changes in the content, style or any other contribution you can think of for this pull request.

Context: Quarkus 3.14 Update Documentation
Where: Migration Guide

If there is anything you'd like to change, feel free to add your suggestions.

Thank you in advance,
Petros

Copy link

@conceptualshark conceptualshark left a comment

Choose a reason for hiding this comment

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

Some minor non-blocking suggestions - otherwise this looks good to me from a docs perspective!

content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
content/update/minor/721-to-722/_index.md Outdated Show resolved Hide resolved
@psavidis psavidis merged commit e557137 into master Sep 12, 2024
1 check passed
@psavidis psavidis deleted the 4163-update-quarkus-314-properties-docs branch September 12, 2024 12:59
@psavidis psavidis removed the request for review from christinaausley September 12, 2024 12:59

- `quarkus.camunda.enforce-history-time-to-live` **becomes** `quarkus.camunda.generic-config.enforce-history-time-to-live`

- `quarkus.camunda.job-executor.thread-pool.max-pool-size` **becomes** `quarkus.camunda.job-executor.generic-config.thread-pool.max-pool-size`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @psavidis ,

I was checking these changes, and I may be missing something, but isn't this incorrect?
Only non specific properties that are mapped to the genericConfig map should be impacted. This shouldn't include properties:

  • job-executor.thread-pool.max-pool-size
  • job-executor.thread-pool.queue-size

We can see here that these two properties haven't changed.

Copy link
Contributor Author

@psavidis psavidis Sep 17, 2024

Choose a reason for hiding this comment

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

The above is correct 👍
Indeed the job executor properties are a poor selection for demonstrating the change in the docs

Proof: Changing the values in the test file mixed-application.properties makes CamundaEngineConfigFileTest#L70 fail which means the properties are mapped correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking. In that case changes in file configuration.md are also incorrect. I can correct these in a new PR to prepare for the 3.15 upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised this PR for addressing the corrections.

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.

4 participants