-
Notifications
You must be signed in to change notification settings - Fork 3
Update API host URL to prod in reference #433
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
Conversation
|
Question: why is this a 65k line PR |
nrichers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@validbeck thank you for making this update! Before I review more thoroughly, some changes are needed:
- Remove the defunct pdoc HTML output files, they are no longer being generated (and they would have been generated by a workflow anyway)
- Remove the QMD files, as they get generated and committed as part of quarto-docs.yaml which runs on its own
Note the Quarto workflow appears to have failed with this error for some time, likely because of a branch protection that was added. I'll follow up.
remote: error: GH006: Protected branch update failed for refs/heads/main.
remote:
remote: - Changes must be made through a pull request.
error: failed to push some refs to 'https://github.com/validmind/validmind-library'
|
|
|
@nibalizer related to this Slack DM, I changed the workflow that is causing issues to create PRs instead of committing directly to EDIT: afff438 |
Gotcha, I wasn't sure on either of these since the READMEs don't seem to have been updated to reflect these processes are covered — can we all maybe a callout or something to both the top-level README and the |
28f7378 to
2d72f1c
Compare
|
@nrichers Sorry, it wouldn't let me checkout |
We do have this in the docs/ readme:
I guess we could be more explicit but since no user interaction is needed, there aren't really steps to follow? |
nrichers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@validbeck your changes LGTM 🚀 but the workflow changes I pushed need input from Spencer before we can call them done (either remove the auto-merge lines and do stuff more manually OR enable auto-merge on the repo).
Proving yet again that @validbeck not only writes excellent docs but also reads them. Removed in b6c7088 |
Thank you — I wasn't sure if I was just lost! Maybe we can put a little note in the docs/README that when you test the generation locally you shouldn't commit the files because that's taken care of by the workflow. EDIT: Actually, why are the builds committed to the |
PR SummaryThis PR introduces two main categories of changes:
Additionally, the .gitignore has been updated to ignore HTML and QMD files, likely to prevent auto-generated or intermediary files from being committed. Test Suggestions
|
nrichers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@validbeck approved - as discussed, I added HTML and QMD files to the .gitignore and commented out the workflow step that requires auto-merge to be enabled. Feel free to merge at your leisure.

Pull Request Description
What and why?
Will resolve the issue noted by @nibalizer in this Slack thread when merged and changes pulled into docs.
How to test
I pulled in this branch into docs and rendered the built API docs again:
What needs special review?
make docsstill throws weird errors for me locally, but everything should be there. . .make quarto-docsworked fine, but please double-check just in case?Dependencies, breaking changes, and deployment notes
Release notes
Checklist