-
Notifications
You must be signed in to change notification settings - Fork 610
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: use algolia for searchability #8410
Conversation
ACTION NEEDED Ibis follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
workflow_dispatch: | ||
repository_dispatch: | ||
schedule: | ||
- cron: "0 0 * * *" |
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.
Should we run this in ibis-docs-main.yml
instead?
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.
I guess we could, this doesn't take to long to run. was keeping it separate because if this fails, it wouldn't messed with the docs build necessarily but, I don't have a strong opinion. I can move it, if you'd prefer so.
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.
I think we should move it so that we get CI failures if the script fails instead of it silently failing and us forgetting to look it at for a long time.
We can determine what to do depending on the nature of the failures if/when they happen.
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.
@cpcloud Do you have a preference here, happy either way?
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.
Oh, sorry yeah. I'd prefer if we moved it to the ibis-docs-main
workflow if you don't mind.
.github/workflows/upload-algolia.py
Outdated
api_key = os.getenv("ALGOLIA_API_KEY") | ||
app_id = os.getenv("ALGOLIA_APP_ID") | ||
index_name = "test_IBIS" # os.getenv("ALGOLIA_INDEX") | ||
index_file = os.getenv("QUARTO_INDEX_PATH") # search.json |
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.
can we use os.environ
instead of the lower-level os.getenv
?
.github/workflows/upload-algolia.py
Outdated
# Download the index generated by quarto from the ibis website | ||
with urlopen("https://ibis-project.org/search.json") as response: | ||
with open(index_file, "wb") as f: | ||
f.write(response.read()) |
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.
Do we actually need to write to disk here? How about calling json.load(f)
directly instead of writing and then reading?
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.
Actually, how about json.loads(response.read().decode("utf-8"))
?
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.
I was trying to convert this ruby code from quarto to python
https://github.com/quarto-dev/quarto-web/blob/main/.github/workflows/upload-algolia.rb
but I think you are right, I'll give that a try.
I'm not sure I understand what's wrong with the PR title, that ci is complaining about. I check the ci job, but it's unclear to me what's happening. |
There's a leading whitespace per https://github.com/ibis-project/ibis/actions/runs/7993226046/job/21828534190?pr=8410#step:5:38 |
I think others have also experienced this issue. Is there some tool that's prepending and/or appending whitespace to commit headers? I don't ever experience this issue using |
This is the first time that happens to me, and I use vim for commits, I did edit the title of the PR, I might have accidentally introduced a space, I have no clue 🤷♀️ |
Merging so we can iterate. |
Favor this PR instead of #8325
Closes: #7995
Needs to happen:
prod_IBIS
To be able to know if this works, idk if the docs-preview label would be enough or if we will have to try this from a branch, due to the secret.