Skip to content

chore(fix): Installing mkdocs related decencies from docs.yml #402

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

siddharth-khatsuriya
Copy link
Contributor

@siddharth-khatsuriya siddharth-khatsuriya commented May 15, 2025

Description

Add-ons can define different sets of MkDocs plugins in the plugins section of their mkdocs.ymlfiles. In order to successfully validate and build the documentation for any given add-on, all the plugin dependencies listed must be installed beforehand.

This PR introduces logic to dynamically parse the mkdocs.yml file, extract the plugin names specified, and install the corresponding Python packages at runtime. This ensures that the mkdocs build command can run successfully for any add-on without requiring manual intervention or hardcoded dependencies.

Checklist

  • README.md has been updated or is not required
  • push trigger tests
  • manual release test
  • automated releases test
  • pull request trigger tests
  • schedule trigger tests
  • workflow errors/warnings reviewed and addressed

Testing done for validate-docs-change and enforce-docs-change for different add-ons

google-workspace: https://github.com/splunk/splunk-add-on-for-google-workspace/actions/runs/15015127274
MSCS: https://github.com/splunk/splunk-add-on-for-microsoft-cloud-services/actions/runs/15037460713
Salesforce: https://github.com/splunk/splunk-add-on-for-salesforce/actions/runs/15037974955

@siddharth-khatsuriya siddharth-khatsuriya force-pushed the test/validate-docs-change branch 2 times, most recently from b69db9f to f854809 Compare May 15, 2025 05:52
@siddharth-khatsuriya siddharth-khatsuriya force-pushed the test/validate-docs-change branch from f854809 to 096ff6c Compare May 15, 2025 06:59
@siddharth-khatsuriya siddharth-khatsuriya marked this pull request as ready for review May 15, 2025 07:08
@siddharth-khatsuriya siddharth-khatsuriya requested a review from a team as a code owner May 15, 2025 07:08
Copy link
Member

@artemrys artemrys left a comment

Choose a reason for hiding this comment

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

Should this be a part of the docs workflow in the first place?

@siddharth-khatsuriya
Copy link
Contributor Author

Should this be a part of the docs workflow in the first place?

Hey I agree, but few factors such as

  • docs.yaml maintained by respective teams and not via workflow-addon-release
  • docs.yaml not running on each pr ( can easily be mitigated, but the first point makes it a bit tricky )
  • right now the only check that is required for merge is in build-test-release ( i.e. pre-publish ), we we are using to block merge in case the base is main.

these would prove as hindrance to achieve this .

pip install mkdocs==1.6.1 mkdocs-material==9.6.9 poetry
pip install poetry
DOCS_FILE=""
if [[ -f ".github/workflows/docs.yml" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having to parse the docs.yml workflow file to get the dependencies to install, it make more sense to keep those dependencies in the pyproject.toml file under a specific group (i.e. docs) and then use directly from there to install these dependencies (using poetry install --only docs).
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

Copy link
Member

Choose a reason for hiding this comment

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

to give a bit of context on why it was done this way - so that doc dependencies can be upgraded separately and not to introduce new dependencies for TA teams

those dependencies would need to be added manually for every repository, we can't roll those out because it will mess poetry.lock and devs would need to update it manually which is quite a procedure

Copy link
Contributor

@mkolasinski-splunk mkolasinski-splunk May 15, 2025

Choose a reason for hiding this comment

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

I agree with argument about updating poetry.lock. On the other hand I don't really like current approach where versions are hardcoded in docs.yml workflow:

  1. complex libs retrieval for validation - as in this PR
  2. if certain repo requires different libraries, template rollout will need to handle exceptions.

What about introducing requirements-docs.txt for now and encouraging devs to introduce new group to pyproject.toml as Darshan mentioned?

@artemrys
Copy link
Member

Should this be a part of the docs workflow in the first place?

Hey I agree, but few factors such as

  • docs.yaml maintained by respective teams and not via workflow-addon-release
  • docs.yaml not running on each pr ( can easily be mitigated, but the first point makes it a bit tricky )
  • right now the only check that is required for merge is in build-test-release ( i.e. pre-publish ), we we are using to block merge in case the base is main.

these would prove as hindrance to achieve this .

good points Siddharth, there are trade-offs with the solution that I propose, especially around your 3rd point; theoretically we can have a PR that would be merged even the documentation fails from docs.yaml workflow; should we include it as necessary check before merging to main (that should be a pretty easy change in the template repo)

regarding first point - it's controlled by template repo, this Jira https://splunk.atlassian.net/browse/ADDON-80483 might help us to mitigate this risk if implemented

@mkolasinski-splunk
Copy link
Contributor

What about adding docs.yml to template repository so we can control

Should this be a part of the docs workflow in the first place?

Hey I agree, but few factors such as

  • docs.yaml maintained by respective teams and not via workflow-addon-release
  • docs.yaml not running on each pr ( can easily be mitigated, but the first point makes it a bit tricky )
  • right now the only check that is required for merge is in build-test-release ( i.e. pre-publish ), we we are using to block merge in case the base is main.

these would prove as hindrance to achieve this .

good points Siddharth, there are trade-offs with the solution that I propose, especially around your 3rd point; theoretically we can have a PR that would be merged even the documentation fails from docs.yaml workflow; should we include it as necessary check before merging to main (that should be a pretty easy change in the template repo)

regarding first point - it's controlled by template repo, this Jira https://splunk.atlassian.net/browse/ADDON-80483 might help us to mitigate this risk if implemented

If we want to enforce state of docs.yml, maybe it's better to create small reusable workflow for this purpose?

@mkolasinski-splunk
Copy link
Contributor

mkolasinski-splunk commented May 20, 2025

Discussed this topic with @pawel-dabro and came up with below idea:

  1. modify docs.yml workflow to be run on the same conditions as addonfactory reusable wf + dispatch
  2. modify docs.yml to run mkdocs gh-deploy --force conditionally only on dispatch event
  3. add required check in branch rule for certain job in docs.yml

Above approach will make sure that changes on TA repos are not negatively affecting docs while keeping docs related logic out of addonfactory-reusable-wf.
Also this way we keep dependencies within workflow file for now under the assumption that they are not going to be modified frequently. If that will be the case, sync-diff workflow will report changes and we can adjust the approach accordingly (ie introduce requirements-docs.txt/new group in pyproject.toml)

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.

4 participants