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

docs(config): add descriptions to all configuration variables and tests #682

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

onyedikachi-david
Copy link

@onyedikachi-david onyedikachi-david commented Oct 11, 2024

Closes #40

/claim #40

Changes proposed

  • Added descriptions to all configuration variables in OpalClientConfig
  • Added descriptions to all configuration variables in OpalServerConfig
  • Added descriptions to all configuration variables in OpalCommonConfig
  • Created a test to ensure all configuration variables have descriptions

Check List (Check all the applicable boxes)

  • I sign off on contributing this submission to open-source
  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarised content.
  • The title of my pull request is a short description of the requested changes.

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for opal-docs canceled.

Name Link
🔨 Latest commit f38be07
🔍 Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/670cb6503eea510008cf6b54

@onyedikachi-david
Copy link
Author

The PR is ready for review @gemanor

Copy link
Collaborator

@gemanor gemanor left a comment

Choose a reason for hiding this comment

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

It is a good start, but the descriptions need to be much more descriptive. I left two examples (but there are more). Please fix them, and commit again.

POLICY_STORE_URL = confi.str(
"POLICY_STORE_URL",
"http://localhost:8181",
description="URL of the policy store"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is exactly what written on the key, please explain more...

INLINE_OPA_ENABLED = confi.bool(
"INLINE_OPA_ENABLED",
True,
description="Whether to run OPA inline within OPAL"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even the code comment has more description of this description. The task is to write descriptions of the variables, not just mark them as solved. Please do some understanding work around OPAL, and explain the configuration variables in a concise but descriptive way.

@onyedikachi-david
Copy link
Author

It is a good start, but the descriptions need to be much more descriptive. I left two examples (but there are more). Please fix them, and commit again.

Done.

@onyedikachi-david
Copy link
Author

@gemanor Please review.

@onyedikachi-david
Copy link
Author

Hi @gemanor, I don't know if you have got time to review this.

1 similar comment
@onyedikachi-david
Copy link
Author

Hi @gemanor, I don't know if you have got time to review this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Go over all OPAL's config variables and add descriptions
2 participants