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

Use the PATCH /embedders path on update_embedders_1 #3099

Merged
merged 5 commits into from
Jan 16, 2025

Conversation

ellnix
Copy link
Contributor

@ellnix ellnix commented Jan 14, 2025

Pull Request

Related issue

Fixes #3096

@ellnix ellnix requested a review from a team as a code owner January 14, 2025 12:50
@guimachiavelli
Copy link
Member

Hmmm…for some reason GH is not able to run that action, even though all your previous PRs had no issues. Making the same changes in another PR works, so I'm not sure what is causing the GH action to fail.

I'll take a look at this again tomorrow, in case it's a temporary fluke on GitHub's side.

@ellnix
Copy link
Contributor Author

ellnix commented Jan 14, 2025

Hmmm…for some reason GH is not able to run that action, even though all your previous PRs had no issues. Making the same changes in another PR works, so I'm not sure what is causing the GH action to fail.

I'll take a look at this again tomorrow, in case it's a temporary fluke on GitHub's side.

I don't think any checks ran on my previous PRs, since this is the first PR after you merged my first PR. Technically before this PR you would have to approve running checks since I was a stranger to this repo.

image

It looks to me that the action is broken generally.

@ellnix
Copy link
Contributor Author

ellnix commented Jan 14, 2025

Seems to also be broken on #3102

0s
Run gh pr comment $CURRENT_BRANCH --body-file comment.txt
gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable. Example:
  env:
    GH_TOKEN: ${{ github.token }}

@guimachiavelli
Copy link
Member

Yeah, I think you're right, but at the moment it's very unclear what exactly is broken. I'm guessing something related to the API token we're using and permissions, since the action seems to be failing only for contributors outside the meilisearch org.

I'll continue looking into it and hopefully merge this PR soon.

@guimachiavelli
Copy link
Member

Instead of keeping this on a limbo until we sort out whatever is happening with the repo's workflows, I'm going to bypass the requirements.

The content of the PR looks good and the only relevant action automates something I can do manually.

Thanks for another useful contribution, @ellnix!

@guimachiavelli guimachiavelli merged commit a5f90de into meilisearch:main Jan 16, 2025
1 check failed
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.

update_embedders_1 sample does not use the embedders route
2 participants