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

Added docs for Config and Auth & update doc workflow #435

Merged

Conversation

Bobbins228
Copy link
Contributor

@Bobbins228 Bobbins228 commented Dec 21, 2023

Issue link

Closes #429

What changes have been made

The detailed "code orientated" docs have been moved to a new folder within docs called detailed-docs
The release workflow has been updated to account for this.
New docs were added for Cluster Configuration and Authentication

Ingress instructions in README moved to cluster-configuration.md

Verification steps

You can verify the workflow changes by running this command.

poetry run pdoc --html -o docs/detailed-documentation src/codeflare_sdk && pushd docs/detailed-documentation && rm -rf cluster job utils && mv codeflare_sdk/* . && rm -rf codeflare_sdk && popd && find docs/detailed-documentation -type f -name "*.html" -exec bash -c "echo '' >> {}" \;

You should get a docs/detailed-docs folder containing the detailed documentation.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

Copy link
Contributor

@VanillaSpoon VanillaSpoon left a comment

Choose a reason for hiding this comment

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

Tiny nit, but other than that lgtm, great job @Bobbins228 :)

docs/authentication.md Outdated Show resolved Hide resolved
Copy link
Contributor

@VanillaSpoon VanillaSpoon left a comment

Choose a reason for hiding this comment

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

lgtm :)

Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

Looks great! I left a couple nitpicks.

docs/detailed-documentation/cluster/config.html Outdated Show resolved Hide resolved
docs/detailed-documentation/job/jobs.html Outdated Show resolved Hide resolved
@ChristianZaccaria
Copy link
Collaborator

You might need to rebase as well.

Copy link
Contributor

@Fiona-Waters Fiona-Waters left a comment

Choose a reason for hiding this comment

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

This is great!
Just one comment. I think we should add a section intro to the jobs.html file, so that all sub_modules have one. I think the ... on this page looks like something is missing, especially with the whitespace underneath.
image
What would you think about making these into links so it's clearer where they are coming from? Just a thought.

@Bobbins228
Copy link
Contributor Author

@Fiona-Waters
These html pages are generated off of comments in the code and so the modules jobs, cluster and utils would not generate any notes because they are folders within the SDK. There might be a way to add a description for these modules so that they are generated automatically but I think it would warrant a separate issue WDYT?

@Fiona-Waters
Copy link
Contributor

@Fiona-Waters These html pages are generated off of comments in the code and so the modules jobs, cluster and utils would not generate any notes because they are folders within the SDK. There might be a way to add a description for these modules so that they are generated automatically but I think it would warrant a separate issue WDYT?

Is it not just a matter of adding a comment here like the one included for ray jobs here?

@Bobbins228
Copy link
Contributor Author

Bobbins228 commented Jan 18, 2024

@Fiona-Waters My bad I thought you meant the module yeah I can add a short desc of the DDP Jobs 👍

@Fiona-Waters
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024
Copy link
Contributor

openshift-ci bot commented Jan 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fiona-Waters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit ed9840b into project-codeflare:main Jan 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document required Authentication steps in codeflare-sdk and ClusterConfiguration Parameters
4 participants