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

One AsciiDoc reference file per parameter #19637

Closed
wants to merge 2 commits into from
Closed

One AsciiDoc reference file per parameter #19637

wants to merge 2 commits into from

Conversation

themr0c
Copy link
Contributor

@themr0c themr0c commented Apr 21, 2021

To feed the discussion in #19630

Now:

  • The location of the files is maybe not optimal.
  • This PR doesn't include changes to validate changes on che.properties or multiuser.properties.
  • Added the script that generated the ref files for reference.
  • Added an assembly file, which is not exactly following what we have in che-docs. Ordering need some work.

@che-bot
Copy link
Contributor

che-bot commented Apr 21, 2021

Can one of the admins verify this patch?

@che-bot che-bot added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 21, 2021
@themr0c
Copy link
Contributor Author

themr0c commented Apr 21, 2021

How it would look like in the docs website:

Screenshot from 2021-04-21 17-29-03
Screenshot from 2021-04-21 17-28-49

@skabashnyuk
Copy link
Contributor

Please merge the latest master to fix the build check.

@sparkoo
Copy link
Member

sparkoo commented Apr 26, 2021

default values in adoc files are prone to become obsolete, this is not safe. It's arguable if these default values are really default, because che-operator/helm might set them to something different with their default installation configuration.

@sparkoo
Copy link
Member

sparkoo commented Apr 26, 2021

how are adoc files going to be referenced from che/multiuser.properties ? Is it in the scope of this PR ?

@sparkoo
Copy link
Member

sparkoo commented Apr 26, 2021

why is there a script in this PR? My assumption was that the comments would be transferred in one batch, so why we need the script here? What will be the process.

@themr0c
Copy link
Contributor Author

themr0c commented Apr 26, 2021

default values in adoc files are prone to become obsolete, this is not safe. It's arguable if these default values are really default, because che-operator/helm might set them to something different with their default installation configuration.

  • A change of the defaults in a properties file must be reflected in the documentation. This is the kind of thing we can catch at review time, if we don't manage to set up a scripted check.

  • Default values (for documentation) are taken from the current content in the properties files. As it is in current docs. If we have to document 3 defaults: default, default for che-operator, default for helm, would it make sense to document here the defaults for the operator and for helm? That would build the case for the documentation entirely in che-docs.

@themr0c
Copy link
Contributor Author

themr0c commented Apr 26, 2021

why is there a script in this PR? My assumption was that the comments would be transferred in one batch, so why we need the script here? What will be the process.

Because it's a draft and I fear the content will get obsolete before we reach consensus. Need to remove it before finalizing.

@themr0c
Copy link
Contributor Author

themr0c commented Apr 26, 2021

how are adoc files going to be referenced from che/multiuser.properties ? Is it in the scope of this PR ?

I believe we should do it in 2 different PR, change the comments once the documentation has done the transition.

I believe we could replace comments by:

# See: path/to/file.adoc

@themr0c
Copy link
Contributor Author

themr0c commented Apr 26, 2021

  • Is the path proposed here something that make sense? Any better / more reasonable idea?

@themr0c
Copy link
Contributor Author

themr0c commented Apr 26, 2021

  • New check for PR validation is missing, script here can maybe serve as a starting point.

@themr0c
Copy link
Contributor Author

themr0c commented May 4, 2021

Closing, new plan described here: #19630 (comment)

@themr0c themr0c closed this May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants