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

feat: add custom annotation for configuration property and feature flag documentation #2852

Merged
merged 39 commits into from
Nov 28, 2022

Conversation

ossdhaval
Copy link
Contributor

@ossdhaval ossdhaval commented Nov 3, 2022

Prepare


Description

PR is work in progress and aimed to gather feedback on approach towards automating documentation of configuration properties and feature flags

The problem that we are trying to solve is of missing out on documenting properties and feature flags. Developers often add these elements without updating relevant documentation resulting in the user not being aware of the feature or property setting.

If developers can have a simple annotation to take care of generating the documentation, it will make sure that code and documentation will stay in synch for properties and feature flags.

The proposed approach leverages annotation processing at compile time. It involves

  • A custom annotation
  • A custom annotation processor
  • Applying custom annotation to properties and feature flag fields

A sample of generated markdown looks like below:

image

Tasks:

  • Create custom annotation
  • Create annotation processor
  • Figure out how to process annotation at compile time
  • Generate markdown doc from annotation
  • Integrate with the build system so the markdown doc is generated and committed to jans\docs

Review comments:

  • Make properties in the doc as anchor tags so that they can be directly referred to via link (Mike)
  • Properties should be listed in alphabetical order (Mike)
  • Create a new maven module (Yuriz)
  • Review the annotation name (Yuriz)

Target issue

closes #issue-number-here

Implementation Details


Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

@ossdhaval ossdhaval self-assigned this Nov 3, 2022
@mo-auto mo-auto added comp-jans-core Component affected by issue or PR kind-feature Issue or PR is a new feature request labels Nov 3, 2022
@mo-auto mo-auto added the comp-jans-auth-server Component affected by issue or PR label Nov 4, 2022
@ossdhaval
Copy link
Contributor Author

Adding feedback from @nynymike here

image

@yuriyz
Copy link
Contributor

yuriyz commented Nov 4, 2022

At first I was going to ask why don't use markdown template and substituting, like in https://github.com/kongchen/swagger-maven-plugin (sample generated doc https://raw.githubusercontent.com/kongchen/swagger-maven-example/master/generated/document.html).

But here it seems goal is to generate separate markdown files for each java file and then inject into docs, right?
Own generator gives flexibility like it's done in this PR. Lets introduce separate jans-doc maven module for doc purpose. Also lets take better name. @JansConfigProperty sounds confusing, like some AS configuration (or jans-config-api related configuration). Maybe something like @DocProperty ?

@moabu
Copy link
Member

moabu commented Nov 7, 2022

I think this is good approach but im a bit confused here. I think @yuriyz brings up a good note. We are already adding work to our swagger annotations and the more we can add there the less we have to spread out. Meaning less is more for documentation purposes its easier to serve it from one place. With the automation purpose in mind my question would be if we actually miss anything if we follow one spec instead of doing this? Too many is confusing from the users point of view and the developers too. If there is a huge union between swagger spec that can be generated vs this we should reconsider and continue the efforts with our swagger documentation as is being done with config-api.

#2353

@ossdhaval
Copy link
Contributor Author

ossdhaval commented Nov 8, 2022

@moabu @yuriyz

The purpose here is to make sure documentation about these properties and flags is easily accessible as well as providing developers with a way to do that.

Having these properties documented in our doc site (markdown) has a few advantages:

  • We can control the format of the document. It would be very clear and will have only the things that we need.
  • We can have anchor links to property descriptions. This will allow us to refer to them from other docs.
  • Searchable. If I search for the property/flag name in the documentation site, I'll find it if it is there as markdown file. In case of swagger documentation, we will have to link it to the doc site and that will make property/flag names unsearchable.

About using Swagger spec:

  • I'll need more pointers on this as I have seen swagger being used to document only REST API. While what we have here are private fields from Java classes and Enumerations
  • Also, even though we can do this via Swagger spec, I doubt if we will have a clean UI or the ability to point to a particular property description.

About the current approach:

  • Yes, it is custom, but it is very lightweight. So, it is easy to maintain
  • It is custom, so we have more control.
  • Can generate markdown which can be directly injected into the doc site source.

@moabu
Copy link
Member

moabu commented Nov 9, 2022

We can switch swagger to markdown but with that said if we are planning to move ahead with the above proposal lets agree on the following:

  1. We use swagger for documenting all our apies always. Hence, we don't converge or duplicate items but rather we annotate all the APIs extensivley using swagger annotations.
  2. Anything besides APIs can be documented in the above method. Moreover, as @yuriyz mentioned in his comment the module jans-doc should be introduced and we shouldn't use any overlapping names such as @JansConfigProperty as that's very confusing.

@ossdhaval
Copy link
Contributor Author

ossdhaval commented Nov 9, 2022

I agree. Summarizing documentation approaches.

  • REST API -> Swagger
  • Java API -> javadoc
  • Java API that needs to be part of the Documentation site -> Custom annotation

Raised #2938 to add these as guideline to developer documentation.

I'll create jans-doc module.

@ossdhaval
Copy link
Contributor Author

Let me know if @DocumentedJansProperty is a better name than @JansConfigProperty.

@yuriyz
Copy link
Contributor

yuriyz commented Nov 9, 2022

Just @DocProperty sounds good to me. It will be located in io.jans.docs package, so it's clear it's jans.

@yuriyz
Copy link
Contributor

yuriyz commented Nov 9, 2022

@ossdhaval is it possible to generate swagger spec without REST API stuff. I mean just models? There are different tools that can generate markdown out of swagger. In this case we can continue to use swagger annotation and use markdown in our docs from models. I guess this is what @moabu meant.

cc @pujavs , she may know the answer for generator.

@moabu
Copy link
Member

moabu commented Nov 9, 2022

@ossdhaval is it possible to generate swagger spec without REST API stuff. I mean just models? There are different tools that can generate markdown out of swagger. In this case we can continue to use swagger annotation and use markdown in our docs from models. I guess this is what @moabu meant.

cc @pujavs , she may know the answer for generator.

Yes that's exactly what I meant . We can cover a lot with swagger and easily convert to markdown. There shouldn't be an overlap .

@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

[jans-config-api-parent] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

[notify] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Nov 22, 2022

[jans-linux-setup] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ossdhaval
Copy link
Contributor Author

@yuriyz @shmorri @moabu PR is ready for review with all checks passing and conflicts resolved.

Comment on lines 61 to 76
echo "Generate properties and feature flag documents from elements annotated with @DocFeatureFlag and @DocProperty"

# Compile jans-core to pick-up any changes in annotation processors
mvn -q -f jans-core/pom.xml -DskipTests clean compile install

# Compile modules where classes that use these annotations exist.
# This will generate markdown files under target/classes directory
mvn -q -f jans-auth-server/pom.xml clean compile
mvn -q -f jans-fido2/pom.xml clean compile
mvn -q -f jans-scim/pom.xml clean compile

# Move markdown files to appropriate locations under documentation root 'doc'
mv -f jans-auth-server/model/target/classes/janssenauthserver-properties.md docs/admin/reference/json/properties
mv -f jans-auth-server/model/target/classes/janssenauthserver-feature-flags.md docs/admin/reference/json/feature-flags
mv -f jans-fido2/model/target/classes/fido2-properties.md docs/admin/reference/json/properties
mv -f jans-scim/model/target/classes/scim-properties.md docs/admin/reference/json/properties
Copy link
Member

Choose a reason for hiding this comment

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

please inject a bash script at https://github.com/JanssenProject/jans/tree/main/automation under a new folder docs. This will allow the workflows to be a little bit cleaner as I suspect we will have more of these flows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moabu Done. I have moved these commands to a separate script. Please check.

@yuriyz
Copy link
Contributor

yuriyz commented Nov 22, 2022

@ossdhaval lets fix bugs found by sonar

[jans-core] SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 11 Code Smells

0.0% 0.0% Coverage
8.6% 8.6% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Nov 23, 2022

[jans-core] SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
28.4% 28.4% Duplication

@ossdhaval
Copy link
Contributor Author

@ossdhaval lets fix bugs found by sonar

[jans-core] SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs Vulnerability A 0 Vulnerabilities Security Hotspot E 2 Security Hotspots Code Smell A 11 Code Smells

0.0% 0.0% Coverage 8.6% 8.6% Duplication

@yuriyz Thanks for highlighting this. I have now taken care of most of these Sonar issues. There are two due to duplicate code. The check is failing due to the same reason. Some amount of this duplicate code is intentional to allow customized formating of the document if needed. One method createAndWriteDoc can still be extracted out to an abstract class. I tried but ran into problems where the compiler couldn't instantiate the child class as an annotation processor. Hence I have left that method as is.

@moabu moabu merged commit 9991d1c into main Nov 28, 2022
@moabu moabu deleted the properties-doc-automation branch November 28, 2022 10:12
jgomer2001 pushed a commit that referenced this pull request Nov 28, 2022
* docs: docker installation (#3027)

* docs: docker installation

* docs: add quick-start page

* docs: readme.md and compose,md made identical

* docs: adjust warning as per github pages syntex

* docs: replace docker with docker compose

* docs: github page identical to compose page

* docs: remove yml file deletion

* docs: fix

* docs: fix helm chart url

* build(deps): bump zeebe-io/backport-action from 0.0.8 to 0.0.9 (#3060)

Bumps [zeebe-io/backport-action](https://github.com/zeebe-io/backport-action) from 0.0.8 to 0.0.9.
- [Release notes](https://github.com/zeebe-io/backport-action/releases)
- [Commits](korthout/backport-action@v0.0.8...v0.0.9)

---
updated-dependencies:
- dependency-name: zeebe-io/backport-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* docs: improve vm install instructions (#3091)

* docs: add sha check instructions for rhel

* docs: add sha check instructions for suse

* docs: formating and proofreading of install docs (#3092)

* feat: add custom annotation for configuration property and feature flag documentation (#2852)

* feat: add custom annotation for prop documentation

* feat: add annotation processor

* feat: annotate properties

* feat: configure annotation processor

* feat: add default value

* feat: add annotation to enum

* feat: add comment

* feat: rename annotation

* feat: rename processor class

* feat: refactor to new core module

* feat: fix test class errors

* feat: rename the module

* feat: add table and details view of content

* feat: sort properties

* feat: change wording - mandatory to required

* feat: add exception handling and logging

* feat: write file under classes output dir

* feat: create output file under target directory

* feat: rename property and file

* feat: create separate annotation for feature flags

* feat: code cleanup

* fix: add description to properties

* fix: add property descriptions from Gluu docs

* fix: add descriptions from Swagger

* fix(fido2): annotate fido config properties

* feat(scim): configure property documentation annotations

* fix: add module name to file and title

* fix: add Feature Flag descriptions

* fix: integrate doc generation with CI

* fix: add tags to generated docs

* fix: create separate sections for properties and flags

* fix: update the artifact version for jans-doc

* fix: contents of markdown files after merge

* ci: remove token req

* fix: sonar issues

* fix: sonar issues

* fix: sonar issues

* fix: move doc generation to shell script

Co-authored-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com>

* ci: use hotspath-storage in quickstart script

* doc: remove redundant API details sections (#3093)

* feat(jans-auth-server): specify minimum acr for clients #343 (#3083)

* feat(jans-auth-server): specify minimum acr for clients #343

* feat(jans-auth-server): added minimum acr properties to dynamic registration #343

* doc(jans-auth-server): added docs and updated swagger with new minimum acr related properties #343

* docs: add kuberentes planning guide initial points

* docs: add kuberentes planning guide initial points

* Update certificates.md (#3096)

* docs: scim logs

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Amro Misbah <amromisba7@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Dhaval D <343411+ossdhaval@users.noreply.github.com>
Co-authored-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com>
Co-authored-by: YuriyZ <yzabrovarniy@gmail.com>
Co-authored-by: mzico <mohib@gluu.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-jans-auth-server Component affected by issue or PR comp-jans-core Component affected by issue or PR kind-feature Issue or PR is a new feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants