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

FISH-788: update docs for directory config source #126

Merged
merged 7 commits into from
Feb 8, 2021

Conversation

poikilotherm
Copy link
Contributor

@poikilotherm poikilotherm commented Jan 28, 2021

Match directory config source docs to current functionality, as payara/Payara#5007 has been merged. 🎉

This doc change aligns the docs with the new implementation of the directory config source for MicroProfile Config API.

Please review and feel free to change to adapt language, conventions etc.

A former pull request #114 has implemented large parts of this, so this is just an update.

…nality.

This doc change aligns the docs with the new implementation of the directory
config source for MicroProfile Config API.

- See PR payara/Payara#5007
- Internal ticket FISH-788

=== Map property names to a sub-directory structure

_Since *5.2020.7* _
_Since *5.2021.1* _
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please change this if necessary. Can be done via GH suggestion comment for example.

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Jan 28, 2021

DO NOT MERGE YET.

Looks like the initialization needs fixing and maybe we need to write a few words about what one needs to do after setting the config dir.

Resolved by payara/Payara#5104

@rdebusscher rdebusscher self-requested a review January 29, 2021 07:09
Copy link
Contributor

@rdebusscher rdebusscher left a comment

Choose a reason for hiding this comment

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

Do not merge this (yet) as the code PR seems to introduce errors during deployment of application. Further investigation is required and it might not be included in 5.2021.1 .

MPCONFIG DirConfigSource: error during setup.

java.io.IOException: Given MPCONFIG directory 'secrets' is no directory, cannot be read or has a leading dot.

@poikilotherm poikilotherm requested a review from OndroMih January 29, 2021 12:32
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Jan 29, 2021

Do not merge this (yet) as the code PR seems to introduce errors during deployment of application. Further investigation is required and it might not be included in 5.2021.1 .

(To be) resolved by payara/Payara#5104

@poikilotherm
Copy link
Contributor Author

poikilotherm commented Jan 29, 2021

@OndroMih @rdebusscher do you want to add some words about the need to restart the server after setting the directory?

Also, should we document how that can be avoided by using a PREBOOT or POSTBOOT script? This config source is pretty much about being used for K8s and containers, so that would be a very handy info for users.

@rdebusscher
Copy link
Contributor

General point is: Add as much and as detailed info as possible in the doc. But main info seems to be added already (as far as my quick check reveals)

Copy link
Contributor

@rdebusscher rdebusscher 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 for me (quick check)

Copy link
Contributor

@AlanRoth AlanRoth left a comment

Choose a reason for hiding this comment

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

Hi @poikilotherm,

Could you change 'server (the DAS)' default value to just 'server-config' - just to keep it consistent with the rest of our docs, sorry I didn't catch it in the first docs PR. Otherwise looks good to merge to me.

@poikilotherm
Copy link
Contributor Author

As requested, I fixed the wording and added some hints about restarting and using the bootscripts when inside containers.

@AlanRoth AlanRoth merged commit 3151160 into payara:master Feb 8, 2021
@poikilotherm
Copy link
Contributor Author

Thx for merging! Glad to see this is done 😄

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