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.

Sign up for GitHub

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

Jump to bottom

feat(config): add support for restrictionKeys #2602

Merged
merged 1 commit into from
Dec 20, 2024
Merged

feat(config): add support for restrictionKeys #2602

merged 1 commit into from
Dec 20, 2024

Conversation

matthieu-crouzet
Copy link
Contributor

Proposed change

As a developer I want to be able to flag a configuration property as sensitive.
It was decided to have an array of restriction, named restrictionKeys for each property, to support potential future use case with different level of restriction based on the role of the user configuring the property value.
This PR includes:

  • the update of the metadata schema
  • the extraction of restrictionKeys from the jsdoc tags @o3rRestrictionKeys
  • a linter to limit the possible keys used with @o3rRestrictionKeys

Related issues

- No issue associated -

@matthieu-crouzet matthieu-crouzet requested review from a team as code owners December 16, 2024 17:39
@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Dec 16, 2024 •
edited
Loading

View your CI Pipeline Execution ↗ for commit b0bc18b.

Command Status Duration Result
nx run-many --target=test-int ✅ Succeeded 33m 51s View ↗
nx run-many --target=test-e2e ✅ Succeeded 2m 29s View ↗
nx run-many --target=build --projects=eslint-pl... ✅ Succeeded <1s View ↗
nx run-many --target=publish --nx-bail --userco... ✅ Succeeded 35s View ↗
nx run-many --target=build ✅ Succeeded 1m 46s View ↗
nx affected --target=lint --base=remotes/origin... ✅ Succeeded 1m 57s View ↗
nx run-many --target=documentation ✅ Succeeded 1m 19s View ↗
nx affected --target=test --cacheDirectory=D:\a... ✅ Succeeded 8s View ↗
Additional runs (3) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2024-12-20 07:42:12 UTC

@codecov Codecov
Copy link

codecov bot commented Dec 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 66.02%. Comparing base (ebd320a) to head (b0bc18b).
Report is 24 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...r-restriction-key-tags/o3r-restriction-key-tags.ts 90.90% 1 Missing and 2 partials ⚠️
Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cpaulve-1A
cpaulve-1A reviewed Dec 17, 2024
View reviewed changes
...3r/eslint-plugin/src/rules/typescript/o3r-restriction-keys-tags/o3r-restriction-keys-tags.ts Outdated
const { loc, value: docText } = comment;
const actualKeys = Array.from(docText.matchAll(/@o3rRestrictionKeys (.*)/g)).map((match) => match[1]);

if (actualKeys.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't actual keys be mandatory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

number of supported keys should be mandatory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change the code to raise issue is the rule is configured with an empty array for supportedKeys

@matthieu-crouzet matthieu-crouzet force-pushed the feat/restriction-keys branch 3 times, most recently from 7617a31 to debe22b Compare December 17, 2024 14:41
@matthieu-crouzet matthieu-crouzet force-pushed the feat/restriction-keys branch 2 times, most recently from aadaf2e to 7e80aed Compare December 17, 2024 15:00
@kpanot
Copy link
Contributor

kpanot commented Dec 17, 2024

Missing documentation in docs/linter (mandatory because targeted by linter message link)

@matthieu-crouzet matthieu-crouzet force-pushed the feat/restriction-keys branch 3 times, most recently from 8d861ce to 34904c6 Compare December 19, 2024 08:12
fpaul-1A
fpaul-1A previously approved these changes Dec 19, 2024
View reviewed changes
@matthieu-crouzet matthieu-crouzet added this pull request to the merge queue Dec 20, 2024
Merged via the queue into main with commit c8834dc Dec 20, 2024
34 of 36 checks passed
@matthieu-crouzet matthieu-crouzet deleted the feat/restriction-keys branch December 20, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpaulve-1A cpaulve-1A cpaulve-1A left review comments

@kpanot kpanot kpanot left review comments

@sdo-1A sdo-1A sdo-1A approved these changes

@fpaul-1A fpaul-1A fpaul-1A approved these changes

@amadeus-pmeneguzzi amadeus-pmeneguzzi amadeus-pmeneguzzi approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request project:cascading-gh-action project:@o3r/components project:@o3r/configuration project:@o3r/eslint-config project:@o3r/eslint-config-otter project:@o3r/eslint-plugin project:@o3r/extractors project:@o3r/showcase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@matthieu-crouzet @kpanot @amadeus-pmeneguzzi @fpaul-1A @cpaulve-1A @sdo-1A