Skip to content

Conversation

@wtrocki
Copy link
Member

@wtrocki wtrocki commented Mar 13, 2025

Proposed changes

Change format of the document to use sections as tables do not support markdown format.
That restricts us to document each rule.
Avoid repeating url to IPA website (private) it is now mentioned on the top once.
Link directly to implementation from the ipa failure giving user more details on the rule specifics.

@wtrocki wtrocki requested a review from a team as a code owner March 13, 2025 11:59

function getIpaRulesetUrl(ipaNumber) {
return `[${ipaNumber}.yaml](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/${ipaNumber}.yaml)`;
return `[http://go/ipa/${ipaNumber}.yaml](http://go/ipa/${ipaNumber}.yaml)`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this link to the IPA Pine docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

We agreed that validator description provides much more value for users than repeated validator description in pine docs. Made fix. let me know what you think now.

Copy link
Collaborator

@lovisaberggren lovisaberggren left a comment

Choose a reason for hiding this comment

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

Nice, looking good! A few nits

### IPA-005

For rule definitions, see [IPA-005.yaml](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/IPA-005.yaml).
Rule is based on [http://go/ipa/IPA-005](http://go/ipa/IPA-005).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Rule is based on [http://go/ipa/IPA-005](http://go/ipa/IPA-005).
Rule is based on [http://go/ipa/IPA-5](http://go/ipa/IPA-5).

Last comment 😅 Unfortunately these golinks for numbers < 10 don't have the 00 in front, so this one is 404 currently

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah.. that is why we had those manually specified. Adding simple logic to cover it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

function formatSeverity(severity) {
switch (severity.toLowerCase()) {
case 'info':
return '![](https://img.shields.io/badge/info-green)';
Copy link
Contributor

Choose a reason for hiding this comment

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

[a11ly]

Suggested change
return '![](https://img.shields.io/badge/info-green)';
return '![info](https://img.shields.io/badge/info-green)';

screen readers will read the text here since images are not accessible, also did we try emojis?
ℹ️ ⚠️ ❌ ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I had them initially mixed with those but we decided to keep those instead. Adding a11ly change

@wtrocki wtrocki requested a review from lovisaberggren March 13, 2025 16:14
Copy link
Contributor

@gssbzn gssbzn left a comment

Choose a reason for hiding this comment

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

thanks for all the work here

@wtrocki wtrocki merged commit f7ddc75 into main Mar 13, 2025
13 checks passed
@wtrocki wtrocki deleted the CLOUDP-305862 branch March 13, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants