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(datasource): All items in the urls array are wrapped between []() character. #31222

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Prtik12
Copy link

@Prtik12 Prtik12 commented Sep 4, 2024

Fixes #30312

Changes

Updated the formatUrls function to correctly format and escape URLs in the documentation. Fixed bad links and ensured that links in const urls are properly validated.

Context

This PR addresses issues with broken links in the documentation and improves URL formatting to prevent build errors and improve the readability of generated documentation.

Replaces #31161

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@Prtik12 Prtik12 changed the title Fix/items in the urls array are wrapped docs(datasource/deb): All items in the urls array are wrapped between []() character. Sep 4, 2024
@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Sep 6, 2024

Hey,

Thanks for going the effort to add names to all the urls. 🙌

You will also need to add a test to make sure that all the urls in the versioning & manager module have are wrapped in []() chars.

For this, we will need a test in the modules/versioning/index.spec.ts file (same for managers modules).

Since, this is your first time contributing it could be tiring to figure out how the tests work. Here's an example for the versioning module. Based on it, you can try to write on for the maangers module too.

Add this to the modules/versioning/index.spec.ts file. After this line:

const urlRegex = regEx(/^\[.*\]\(.*\)$/);
    for (const [verName, verModule] of Object.entries(loadedVers)) {
      const urls = (verModule as { urls?: string[] }).urls ?? [];
      for (const url of urls) {
        const urlValid = urlRegex.test(url);
        expect({
          versioning: verName,
          urlValid,
        }).toMatchObject({
          versioning: verName,
          urlValid: true,
        });
      }
    }

It goes through each versioning module, takes the urls field and uses regex matching to check if url is wrapped in []() chars.

@Prtik12
Copy link
Author

Prtik12 commented Sep 7, 2024

Screenshot 2024-09-07 at 6 06 54 PM

Hi,
I recently ran the test suite and noticed that one of the tests was skipped. I wanted to ask if the skipped test could have any potential impact on the project or if there are any known issues related to it. Should i be concerned about this, or is there a reason why it's skipped that i should be aware of?

Thanks for your assistance!

@viceice viceice changed the title docs(datasource/deb): All items in the urls array are wrapped between []() character. docs(datasource): All items in the urls array are wrapped between []() character. Sep 7, 2024
Comment on lines 150 to 151
'image.toolkit.fluxcd.io/v1beta2',
],
Copy link
Member

Choose a reason for hiding this comment

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

revert and do in separate PR

const urlRegex = regEx(/^\[.*\]\(.*\)$/);

for (const [managerName, managerModule] of Object.entries(loadedMgr)) {
const urls = (managerModule as { urls?: string[] }).urls ?? [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const urls = (managerModule as { urls?: string[] }).urls ?? [];
const urls = (managerModule as { urls?: string[] })?.urls ?? [];

why the cast? 🤔 we've an interface for them, see docs generation inside tools folder

Copy link
Collaborator

@RahulGautamSingh RahulGautamSingh Sep 9, 2024

Choose a reason for hiding this comment

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

Its not needed for the manager module because ManagerApi extends ModuleApi which has the preoperty urls.

But, the VersioningApi is stand-alone and does not extend the ModuleApi, so it doesn't allow the urls property.
Needs to be fixed in diff PR, first Or we could use the Versioning type defined in the tools/docs/versioning.ts


const urlRegex = regEx(/^\[.*\]\(.*\)$/);
for (const [verName, verModule] of Object.entries(loadedVers)) {
const urls = (verModule as { urls?: string[] }).urls ?? [];
Copy link
Member

Choose a reason for hiding this comment

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

see other comments

const urlRegex = regEx(/^\[.*\]\(.*\)$/);

for (const [managerName, managerModule] of Object.entries(loadedMgr)) {
const urls = (managerModule as { urls?: string[] }).urls ?? [];
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
const urls = (managerModule as { urls?: string[] }).urls ?? [];
const urls = managerModule?.urls ?? [];

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.

Replace bare links in index.ts files in the modules/versioning/*/ pattern with good links
3 participants