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

fix!(jans-cedarling): fix cedarling-properties docs to be more proper #10788

Merged
merged 14 commits into from
Feb 4, 2025

Conversation

olehbozhok
Copy link
Contributor

Prepare


Description

Improve documentation. Fix name of some keys. Added information about required bootstrap properties keys.

Target issue

link

closes #10748

Implementation Details

  • align config parameter names against Cedarling-Nativity-Plan
  • add section Required keys for startup
  • now if key CEDARLING_TOKEN_CONFIGS is absent serde show key in error

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)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Because it hard to keep consistent, and contains mistakes

Signed-off-by: Oleh Bohzok <olehbozhok@gmail.com>
(cherry picked from commit cdd7a41)
…Nativity-Plan

Signed-off-by: Oleh Bohzok <olehbozhok@gmail.com>
Signed-off-by: Oleh Bohzok <olehbozhok@gmail.com>
Signed-off-by: Oleh Bohzok <olehbozhok@gmail.com>
Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
To highlight key if missing

Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
@mo-auto mo-auto added area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-bug Issue or PR is a bug in existing functionality labels Feb 4, 2025
@@ -152,7 +152,7 @@ pub struct BootstrapConfigRaw {

/// Configuration for token-based entities, mapping token names to their
/// respective settings.
#[serde(rename = "CEDARLING_TOKEN_CONFIGS", default)]
#[serde(rename = "CEDARLING_TOKEN_CONFIGS")]
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the serde(default) here? won't the deserialization fail if the CEDARLING_TOKEN_CONFIGS is not provided? the TokenConfigs struct implements Default in raw_config/token_settings.rs.

Are we requiring the user to set this bootstrap config now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I missed that TokenConfigs have custom implementation of Default trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 407f5d8
and added doc a6052ed

@rmarinn
Copy link
Contributor

rmarinn commented Feb 4, 2025

it also looks like python tests are failing now. might be because the names of some bootstrap properties were changed.

…ING_TOKEN_CONFIGS` when parsing wia serde

Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
…that show default implementation of `CEDARLING_TOKEN_CONFIGS`

Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
…LING_POLICY_STORE_LOCAL`

Signed-off-by: Oleh Bozhok <olehbozhok@gmail.com>
@SafinWasi SafinWasi changed the title fix(jans-cedarling): fix cedarling-properties docs to be more proper fix!(jans-cedarling): fix cedarling-properties docs to be more proper Feb 4, 2025
Copy link
Contributor

@SafinWasi SafinWasi 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 to me. I changed the PR title a bit to include breaking change !

@olehbozhok olehbozhok merged commit 4b4d5b2 into main Feb 4, 2025
7 checks passed
@olehbozhok olehbozhok deleted the jans-cedaling-issue-10748 branch February 4, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-bug Issue or PR is a bug in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(jans-cedarling): fix cedarling-properties docs to be more proper
5 participants