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

Move doc files from main repo into docs repo #426

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

voetberg
Copy link
Contributor

View issue rucio/rucio#7278 ; Removing check_rest_api_documentation from that repo and moving here as part of decommissioning of the docs test suite

@voetberg voetberg force-pushed the rucio-7278-remove-doc-files branch from f1cfcdd to 3d08aaf Compare December 17, 2024 15:27
@@ -69,7 +69,7 @@ filtered and produces an error, if found.
suite. To manually check the generated spec file, run

```bash
rucio/tools/test/check_rest_api_documentation.sh FILE
rucio_documentation/tools/check_rest_api_documentation.sh FILE
Copy link
Contributor

@rdimaio rdimaio Dec 18, 2024

Choose a reason for hiding this comment

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

Maybe we should say this to be explicit

Suggested change
rucio_documentation/tools/check_rest_api_documentation.sh FILE
[rucio documentation folder]/tools/check_rest_api_documentation.sh FILE

rucio_documentation is clear enough imo, but it might be interpreted as if we have an actual folder named rucio_documentation, whereas [rucio documentation folder] might be more easily interpreted as 'top-level Rucio documentation folder'

Copy link
Contributor Author

@voetberg voetberg Dec 18, 2024

Choose a reason for hiding this comment

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

If we're linking directly to the file anyways, is there any harm in going Full explicit and doing

Suggested change
rucio_documentation/tools/check_rest_api_documentation.sh FILE
[rucio/documentation](https://github.com/rucio/documentation)/tools/check_rest_api_documentation.sh FILE

?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea, but I feel that the / in rucio/documentation could be interpreted as being part of the path, so it might be confusing

Comment on lines +16 to +19
from apispec import APISpec # type: ignore
from apispec_webframeworks.flask import FlaskPlugin # type: ignore
from rucio.vcsversion import VERSION_INFO # type: ignore
from rucio.web.rest.flaskapi.v1.main import application # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the # type: ignore comments for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The python script checking action claims these packages don't exist and fails the build 😔

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add them to the requirements instead - I guess they can be seen as part of the requirements for building the docs, so it makes sense that they'd need to be part of the requirements.txt file

  check_python_scripts:
    name: Check Python Scripts
    runs-on: ubuntu-latest
    steps:
      - name: Checkout repository
        uses: actions/checkout@v4
      - name: Install dependencies
        run: |
          pip install -r tools/requirements.txt
      - name: Run black
        run: |
          black --check .
      - name: Run isort
        run: |
          isort --profile black --filter-files --check .
      - name: Run flake8
        run: |
          flake8 --config tools/.flake8
      - name: MyPy
        run: |
          mypy --ignore-missing-imports .

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.

2 participants